Bump pytest from 5.4.3 to 7.0.1

[Riatre]

Why are these changes needed?

See #23676 for context. This is another attempt at that as I figured out what's going wrong in bazel test. Supersedes #24828.

Now that there are Python 3.10 wheels for Ray 1.13 and this is no longer a blocker for supporting Python 3.10, I still want to make bazel test //python/ray/tests/... work for developing in a 3.10 env, and make it easier to add Python 3.10 tests to CI in future.

The change contains three commits with rather descriptive commit message, which I repeat here:

Pass deps to py_test in py_test_module_list

Bazel macro py_test_module_list takes a `deps` argument, but completely
ignores it instead of passes it to `native.py_test`. Fixing that as we
are going to use deps of py_test_module_list in BUILD in later changes.

cpp/BUILD.bazel depends on the broken behaviour: it deps-on a cc_library
from a py_test, which isn't working, see upstream issue:
https://github.com/bazelbuild/bazel/issues/701.
This is fixed by simply removing the (non-working) deps.

Depend on conftest and data files in Python tests BUILD files

Bazel requires that all the files used in a test run should be
represented in the transitive dependencies specified for the test
target. For py_test, it means srcs, deps and data.

Bazel enforces this constraint by creating a "runfiles" directory,
symbolic links files in the dependency closure and run the test in the
"runfiles" directory, so that the test shouldn't see files not in the
dependency graph.

Unfortunately, the constraint does not apply for a large number of
Python tests, due to pytest (>=3.9.0, <6.0) resolving these symbolic
links during test collection and effectively "breaks out" of the
runfiles tree.

pytest >= 6.0 introduces a breaking change and removed the symbolic link
resolving behaviour, see pytest pull request
https://github.com/pytest-dev/pytest/pull/6523 for more context.

Currently, we are underspecifying dependencies in a lot of BUILD files
and thus blocking us from updating to newer pytest (for Python 3.10
support). This change hopefully fixes all of them, and at least those in
CI, by adding data or source dependencies (mostly for conftest.py-s)
where needed.

Bump pytest version from 5.4.3 to 7.0.1

We want at least pytest 6.2.5 for Python 3.10 support, but not past
7.1.0 since it drops Python 3.6 support (which Ray still supports), thus
the version constraint is set to <7.1.

Updating pytest, combined with earlier BUILD fixes, changed the ground
truth of a few error message based unit test, these tests are updated to
reflect the change.

There are also two small drive-by changes for making test_traceback and
test_cli pass under Python 3.10. These are discovered while debugging CI
failures (on earlier Python) with a Python 3.10 install locally.  Expect
more such issues when adding Python 3.10 to CI.

Related issue number

Closes #23675

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 :(

[Riatre]

CI is happy with the change now. The remaining two macOS test failures looks flakey to me.

[acxz]

Now that there are Python 3.10 wheels for Ray 1.13 and this is no longer a blocker for supporting Python 3.10

I would like to clarify that this is still a blocker for python 3.10. Only the CPU wheels have been published, GPU wheels still do not work.

[scv119]

This is awesome, thanks for contributing to ray!

[clarkzinzow]

Thank you for the contribution! One question I have is about the python/ray/tests/confest.py being properly linked as a dependency for tests in non-core libraries, i.e. python/ray/**/tests, such as python/ray/data/tests. Should we have an explicit dependency on python/ray/tests/conftest.py for each of the sub-library test suites?

[clarkzinzow]

Globs are local to the corresponding package, so this is intended to match /python/ray/data/tests/conftest.py, right?

[Riatre]

Yes. The old code looks like copy-paste from //python/ray/tests, it could be more explicit.

[clarkzinzow]

An alternative way that I could see this working is having a single confest target in the root directory that takes care of both.

py_library(
  name = "conftest",
  srcs = ["//python/ray/tests/conftest.py"] + glob(["**/conftest.py"]),
  visibility = [
    "//python/ray/tests:__subpackages__",
    "//python/ray/dashboard:__pkg__",
    "//python/ray/data:__pkg__",
  ],
)

Other libraries can then depend on //:conftest can get both the root conftest and their package-specific conftest.

What do you think about that alternative? Too implicit?

[Riatre]

IIRC this doesn't work. Blaze hides files in subpackages from parent BUILD files (see https://bazel.build/reference/be/functions#subpackages:~:text=Labels%20are%20not%20allowed%20to%20cross%20the%20package%20boundary, hence "globs are local"). Since we have a BUILD at python/ray/tests/BUILD, we have to explicitly expose these conftest files via filegroup in order to use them in the outermost BUILD.bazel.

[clarkzinzow]

What about the libraries that depend on these fixtures in conftest, e.g. python/ray/data/tests?

[Riatre]

Thanks for pointing out this. I've taken a closer look at it and //python/ray/data:tests/test_* should not be working at all (missing the deps to //python/ray/tests:conftest. 🤔 I'm investigating why it passed on CI.

[Riatre]

Ugh. CI runs pip install -e ., which creates a symbolic link from inside site-packages to the working copy. With this it is possible to import everything without making Bazel aware of these dependencies. Other changes in this PR is needed because they didn't import from conftest-s, and pytest (rightfully) isn't recursing into site-packages to collect fixtures, cause they are usually considered system dependencies. OTOH if it is explicitly import-ed in the tests pytest happily uses them.

Anyway, I've added explicit conftest target and deps to python/ray/data/BUILD to make it more clear.

The issue seems more widespread though, for example if you run a Dataset test with bazel test, edit python/ray/data/dataset.py, and bazel test again, bazel decides to NOT rerunning the test because it isn't seeing the dependency between tests and sources. There is a //:ray_lib target documented as:

# This is a dummy test dependency that causes the python tests to be
# re-run if any of these files changes.
py_library(
    name = "ray_lib",
    srcs = glob(
        ["python/ray/**/*.py"],
        exclude = ["python/ray/tests/*.py"],
    ),
    visibility = ["__subpackages__"],
)

But, as you have noted, glob won't match across package boundary so it doesn't catch files under python/ray/data.

I guess bazel test (with caches) does not work for daily development and is only used in CI then.

[clarkzinzow]

That makes perfect sense even if quite an unfortunate state of things, thank you for digging into that and for the great summary!

Anyway, I've added explicit conftest target and deps to python/ray/data/BUILD to make it more clear.

That sounds good, thank you for making that dependency explicit even if it's not currently required.

I guess bazel test (with caches) does not work for daily development and is only used in CI then.

Yep, our current Bazel setup is not set up as it should be, I've mentioned this internally in the past. Since most contributors use pip install -e . + pytest locally, when developing, there hasn't been a lot of motivation to make the local (or general) Bazel story better, although I would personally like to see it get there to align the local and CI testing stories, since this divergence makes it easy for discrepancies/bugs to creep in.

[pcmoritz]

Looks like the tests are passing now, any objections to merging this?

[acxz]

Closes #23675

This PR does not close the above issue as the latest pytest is 7.1.2.

[clarkzinzow]

@acxz We can't upgrade beyond 7.0.1 until we drop Python 3.6 support, since pytest drops 3.6 support in 7.1.0: https://docs.pytest.org/en/7.1.x/changelog.html#pytest-7-1-0-2022-03-13

Doesn't 7.0.1 suffice for Python 3.10? Python 3.10 support was added in pytest 6.2.5: https://docs.pytest.org/en/7.1.x/changelog.html#pytest-6-2-5-2021-08-29

[acxz]

since pytest drops 3.6 support in 7.1.0

got it

Python 3.10 support was added in pytest 6.2.5

Thanks for reminding me!

I guess I was being a bit pedantic, since the issue this PR claims to close still stand notwithstanding 3.10 support.