Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI] Run doc notebooks in CI #24816

Merged
merged 20 commits into from
May 17, 2022
Merged

Conversation

Yard1
Copy link
Member

@Yard1 Yard1 commented May 15, 2022

Why are these changes needed?

Currently, we are not running doc notebooks in CI due to a bazel misconfiguration - we are using glob in a top level package in order to get the paths for the notebooks, but those are contained inside subpackages, which glob purposefully ignores. Therefore, the lists of notebooks to run are empty. This PR fixes that by:

  • Running the py_test_run_all_notebooks macro inside the relevant subpackages
  • Editing the test_myst_doc.py script to allow for recursive search for the target file, allowing to deal with mismatches between name and data arguments in py_test_run_all_notebooks
  • Setting the allow_empty=False flag inside glob calls in our macros to ensure that this oversight is caught early
  • Enabling detection of changes in doc folder for *.ipynb and BUILD files

This PR also adds a GPU runner for doc tests, allowing one of our examples to pass - and setting the infra for more to come. Finally, a misconfigured path for one set of doc tests is also fixed.

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@Yard1 Yard1 changed the title [CI] Run doc notebooks [CI] Run doc notebooks in CI May 16, 2022
@Yard1 Yard1 marked this pull request as ready for review May 16, 2022 21:28
@@ -188,13 +168,13 @@ py_test_run_all_subdirectory(


# --------------------------------------------------------------------
# Test all doc/source/ray-overview/doc_code snippets, used on ray.io
# Test all doc/source/ray-overview/doc_test snippets, used on ray.io
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch!

Copy link
Contributor

@maxpumperla maxpumperla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks so much for catching and fixing this!

@krfricke krfricke merged commit c74886a into ray-project:master May 17, 2022
@Yard1 Yard1 deleted the run_doc_notebooks branch May 17, 2022 09:04
maxpumperla pushed a commit that referenced this pull request May 18, 2022
Currently, we are not running doc notebooks in CI due to a bazel misconfiguration - we are using `glob` in a top level package in order to get the paths for the notebooks, but those are contained inside subpackages, which glob purposefully ignores. Therefore, the lists of notebooks to run are empty. This PR fixes that by:
* Running the `py_test_run_all_notebooks` macro inside the relevant subpackages
* Editing the `test_myst_doc.py` script to allow for recursive search for the target file, allowing to deal with mismatches between `name` and `data` arguments in `py_test_run_all_notebooks`
* Setting the `allow_empty=False` flag inside `glob` calls in our macros to ensure that this oversight is caught early
* Enabling detection of changes in doc folder for `*.ipynb` and `BUILD` files

This PR also adds a GPU runner for doc tests, allowing one of our examples to pass - and setting the infra for more to come. Finally, a misconfigured path for one set of doc tests is also fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants