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

[core] Fail OSS CI on pytest warnings #31479

Open
cadedaniel opened this issue Jan 5, 2023 · 1 comment
Open

[core] Fail OSS CI on pytest warnings #31479

cadedaniel opened this issue Jan 5, 2023 · 1 comment
Assignees
@cadedaniel
Labels
core Issues that should be addressed in Ray Core Devprod P1 Issue that should be fixed within a few weeks

Comments

@cadedaniel
Copy link
Member

cadedaniel commented Jan 5, 2023 •
edited

Per https://docs.google.com/document/d/1TVMfmhO0vD1MdkVrqRS9t4om2shMTY9nqdqqOopNv0M/edit#, fail CI on pytest warnings. This will enable us to catch DeprecationWarnings before they're deprecated, as well as highlight any warnings thrown by downstream libraries.

PRs:
@cadedaniel cadedaniel self-assigned this Jan 5, 2023
@cadedaniel cadedaniel changed the title Fail OSS CI on pytest warnings [core] Fail OSS CI on pytest warnings Jan 5, 2023
@cadedaniel cadedaniel added core Issues that should be addressed in Ray Core P1 Issue that should be fixed within a few weeks labels Jan 5, 2023
@cadedaniel cadedaniel mentioned this issue Jan 5, 2023
Closed
11 tasks
@rkooo567
Copy link
Contributor

rkooo567 commented Jan 6, 2023

I feel like the PR is out there for a pretty long time. Is it possible to fix this asap and merge it?

@cadedaniel cadedaniel mentioned this issue Jan 7, 2023
Merged
scv119 pushed a commit that referenced this issue 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>
AmeerHajAli pushed a commit that referenced this issue 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>
@scv119 scv119 added the Devprod label Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cadedaniel cadedaniel

Labels
core Issues that should be addressed in Ray Core Devprod P1 Issue that should be fixed within a few weeks
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Draft] [Fail on pytest warnings 2/n] Failing on pytest warnings cadedaniel/ray
3 participants
@scv119 @cadedaniel @rkooo567