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.

Sign up for GitHub

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

Jump to bottom

[Draft] [Fail on pytest warnings 2/n] Failing on pytest warnings #31219

Closed
wants to merge 2 commits into from
Closed

[Draft] [Fail on pytest warnings 2/n] Failing on pytest warnings #31219

wants to merge 2 commits into from

Conversation

cadedaniel
Copy link
Member

@cadedaniel cadedaniel commented Dec 20, 2022 •
edited
Loading

Background

As part of #31479, we will fail CI on pytest warnings. This will help us catch deprecations earlier and improve test hygiene. In order to do so, we need to first whitelist existing warnings so we can fix them incrementally.

Closes #31479
Further Context: https://docs.google.com/document/d/1TVMfmhO0vD1MdkVrqRS9t4om2shMTY9nqdqqOopNv0M/edit#

Remaining steps:

  • Merge [Fail on pytest warnings 1/n] Marking strings with invalid escape sequences as raw strings #31523
    • This is failing because our PR builds use a pre-built wheel, which has the old docstrings.
    • "SyntaxError: invalid escape sequence "
  • Make sure ray client tests still work -- they have explicit warning assertions
    • test_client_deprecation_warn -- Ray Client has tests that expect warnings to be thrown. Since we change that behavior, we have to change the tests to disable the error behavior.
    • Why is this a RuntimeError / shouldn't this always be thrown?
      • ignore:RuntimeError:Maybe you called ray.init twice by accident?:RuntimeError
      • I think this is the same as test_client_deprecation_warn, or at least related.
  • Handle DeprecationWarning: crc32c.crc32 will be eventually removed, use crc32c.crc32c instead
    • ray.tune.error.TuneError failure caused by warning behavior change
  • resource warnings are not ignored properly (are they wrapped or not?)
  • See if MacOS tests are getting skipped (PytestUnhandledCoroutineWarning) (they are :/)
  • Add filters for new warnings
    • ImportError during pytest parsing of pytest.ini. I think I need to upgrade urllib for python3.10 minimal install
    • tests:test_runtime_env is timing out in py3.11 and py3.9 minimal install
    • terminate called after throwing an instance of 'std::bad_alloc' in //python/ray/train:distributed_sage_example
  • Create GitHub issues for warnings that should be un-ignored, having some impact on our velocity or userbase if we ignore
    • Create issue for: pytest.PytestCollectionWarning: cannot collect test class 'TestCommandTimeout' because it has a init constructor (from: bazel-out/k8-opt/bin/release/test_glue.runfiles/com_github_ray_project_ray/release/ray_release/tests/test_glue.py)

@cadedaniel cadedaniel added core Issues that should be addressed in Ray Core @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Dec 20, 2022
@cadedaniel cadedaniel self-assigned this Dec 20, 2022
@cadedaniel cadedaniel force-pushed the pytest-throw-on-warning branch from d593ee8 to 8149c8a Compare January 5, 2023 19:14
cadedaniel
cadedaniel commented Jan 5, 2023
View reviewed changes
@cadedaniel
Copy link
Member Author

note to self: there seem to be a good bit more warnings than I expected. I might have to put all of them into the pytest.ini at high level and leave for future prioritization instead of fixing in each test

@cadedaniel cadedaniel force-pushed the pytest-throw-on-warning branch 3 times, most recently from d2620cb to 977404a Compare January 7, 2023 20:59
@cadedaniel cadedaniel changed the title [Draft] Failing on pytest warnings [Draft] [Fail on pytest warnings 2/n] Failing on pytest warnings Jan 7, 2023
This was referenced Jan 7, 2023
Open
Merged
@cadedaniel cadedaniel force-pushed the pytest-throw-on-warning branch from bfb66fb to e6d8594 Compare January 8, 2023 03:28
scv119 pushed a commit that referenced this pull request Jan 11, 2023
@cadedaniel
[Fail on pytest warnings 1/n] Marking strings with invalid escape seq…
e54ff46 
…uences as raw strings (#31523)

Background
Python deprecated invalid unicode escape sequences a while back (Python3.6) and they will start breaking in newer Pythons:

Changed in version 3.6: Unrecognized escape sequences produce a DeprecationWarning. In a future Python version they will be a SyntaxWarning and eventually a SyntaxError.

Docstrings and other strings that need \  et al. should be raw strings.

Pytest?
For reasons unclear to me, pytest encounters the SyntaxError when it instruments tests with improved assertions when its warnings are interpreted as failures. It's unclear how to ignore them (because they're marked as SyntaxErrors, which aren't warnings and can't be filtered).

So, in this PR we fix the occurrences where we have invalid escape sequences. We have to do it in a separate PR from #31219 because our CI is using a prebuilt wheel instead of a per-PR wheel.

#31479 tracks the work to fail on pytest warnings.

Example offenders
For example, see lines 406, 407, 412, and 413 here:

ray/python/ray/data/grouped_dataset.py

Lines 406 to 413 in 0c8b59d

             ...     for i in range(100)]) \ # doctest: +SKIP 
             ...     .groupby(lambda x: x[0] % 3) \ # doctest: +SKIP 
             ...     .sum(lambda x: x[2]) # doctest: +SKIP 
             >>> ray.data.range_table(100).groupby("value").sum() # doctest: +SKIP 
             >>> ray.data.from_items([ # doctest: +SKIP 
             ...     {"A": i % 3, "B": i, "C": i**2} # doctest: +SKIP 
             ...     for i in range(100)]) \ # doctest: +SKIP 
             ...     .groupby("A") \ # doctest: +SKIP 

Signed-off-by: Cade Daniel <cade@anyscale.com>
@cadedaniel cadedaniel force-pushed the pytest-throw-on-warning branch 2 times, most recently from d36bc87 to d7e9430 Compare January 12, 2023 02:36
AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023
@cadedaniel @AmeerHajAli
[Fail on pytest warnings 1/n] Marking strings with invalid escape seq…
1f61907 
…uences as raw strings (#31523)

Background
Python deprecated invalid unicode escape sequences a while back (Python3.6) and they will start breaking in newer Pythons:

Changed in version 3.6: Unrecognized escape sequences produce a DeprecationWarning. In a future Python version they will be a SyntaxWarning and eventually a SyntaxError.

Docstrings and other strings that need \  et al. should be raw strings.

Pytest?
For reasons unclear to me, pytest encounters the SyntaxError when it instruments tests with improved assertions when its warnings are interpreted as failures. It's unclear how to ignore them (because they're marked as SyntaxErrors, which aren't warnings and can't be filtered).

So, in this PR we fix the occurrences where we have invalid escape sequences. We have to do it in a separate PR from #31219 because our CI is using a prebuilt wheel instead of a per-PR wheel.

#31479 tracks the work to fail on pytest warnings.

Example offenders
For example, see lines 406, 407, 412, and 413 here:

ray/python/ray/data/grouped_dataset.py

Lines 406 to 413 in 0c8b59d

             ...     for i in range(100)]) \ # doctest: +SKIP 
             ...     .groupby(lambda x: x[0] % 3) \ # doctest: +SKIP 
             ...     .sum(lambda x: x[2]) # doctest: +SKIP 
             >>> ray.data.range_table(100).groupby("value").sum() # doctest: +SKIP 
             >>> ray.data.from_items([ # doctest: +SKIP 
             ...     {"A": i % 3, "B": i, "C": i**2} # doctest: +SKIP 
             ...     for i in range(100)]) \ # doctest: +SKIP 
             ...     .groupby("A") \ # doctest: +SKIP 

Signed-off-by: Cade Daniel <cade@anyscale.com>
@cadedaniel cadedaniel force-pushed the pytest-throw-on-warning branch from 2f32407 to 13f0d90 Compare January 13, 2023 00:56
cadedaniel
cadedaniel commented Jan 13, 2023
View reviewed changes
python/ray/tests/test_runtime_context.py
@@ -331,6 +331,7 @@ def test_no_auto_init(shutdown_only):
assert not ray.is_initialized()


@pytest.mark.filterwarnings("default")
Copy link
Member Author

Choose a reason for hiding this comment

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

do I still need this?

@cadedaniel cadedaniel force-pushed the pytest-throw-on-warning branch 3 times, most recently from b159944 to 07fd7b7 Compare January 24, 2023 02:16
@cadedaniel cadedaniel force-pushed the pytest-throw-on-warning branch from 07fd7b7 to cf2abc2 Compare March 7, 2023 00:47
@cadedaniel
Ignoring existing pytest warnings
ea655ff 
Config file syntax error

Fixing bugs, consolidating

Fixing regex bugs, adding comments

more bugfixes

comment

Temporarily disabling tests which invoke warnings API in case they're disabling pytest.ini

Fixing regex bugs

Documenting failures

Noting pytest warning sources so we can triage

Revert "Temporarily disabling tests which invoke warnings API in case they're disabling pytest.ini"

This reverts commit b845842.

More ignores

Comment

Attempt to ignore ResourceWarning

Fixes

Skipping warnings in test_cli

More warnings

WIP

Marwil raw string

More warnings

Comment to trigger cpp build

Fixes

Try to force full build with noop requirements.txt change

Two more warnings

Fix

Syntax fix

More warnings

ignore

Try suppress runtime env warn

More warnings

Hack to unsupress annotation warnings

Revert "Hack to unsupress annotation warnings"

This reverts commit be28de3d010611204591e8636b28937c4e227649.

Hack to get around deprecated warning recently added

More raw string conversions

Revert "Hack to get around deprecated warning recently added"

This reverts commit 2eec2c1691cffb359e344c7ad772147b3d5b4531.

Ray dep annotation fix

More warnings

Fixes

Fixes

Fixes

Fixes

Fixes

Fixes

Fixes

Fix

Lint

Fix

Fix

Fixes

fix
@cadedaniel
Update
3b25e95
@cadedaniel cadedaniel force-pushed the pytest-throw-on-warning branch from 5b1a4de to 3b25e95 Compare March 14, 2023 23:50
@ollie-iterators
Copy link

I think this is a very important thing that should be merged ASAP.

@ollie-iterators ollie-iterators mentioned this pull request Mar 24, 2023
Open
@cadedaniel cadedaniel closed this Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hongpeng-guo hongpeng-guo Awaiting requested review from hongpeng-guo hongpeng-guo will be requested when the pull request is marked ready for review hongpeng-guo is a code owner

@justinvyu justinvyu Awaiting requested review from justinvyu justinvyu will be requested when the pull request is marked ready for review justinvyu is a code owner

@matthewdeng matthewdeng Awaiting requested review from matthewdeng matthewdeng will be requested when the pull request is marked ready for review matthewdeng is a code owner

@raulchen raulchen Awaiting requested review from raulchen raulchen will be requested when the pull request is marked ready for review raulchen is a code owner

@woshiyyya woshiyyya Awaiting requested review from woshiyyya woshiyyya will be requested when the pull request is marked ready for review woshiyyya is a code owner

@sven1977 sven1977 Awaiting requested review from sven1977 sven1977 will be requested when the pull request is marked ready for review sven1977 is a code owner

@simonsays1980 simonsays1980 Awaiting requested review from simonsays1980 simonsays1980 will be requested when the pull request is marked ready for review simonsays1980 is a code owner

Assignees

@cadedaniel cadedaniel

Labels
@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. core Issues that should be addressed in Ray Core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[core] Fail OSS CI on pytest warnings
2 participants
@cadedaniel @ollie-iterators