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

[Fail on pytest warnings 1/n] Marking strings with invalid escape sequences as raw strings #31523

Merged
merged 4 commits into from
Jan 11, 2023
Merged

[Fail on pytest warnings 1/n] Marking strings with invalid escape sequences as raw strings #31523

merged 4 commits into from
Jan 11, 2023

Conversation

cadedaniel
Copy link
Member

@cadedaniel cadedaniel commented Jan 7, 2023 •
edited
Loading

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

@cadedaniel
Marking strings with invalid escape sequences as raw strings
b0cd0c7 
Signed-off-by: Cade Daniel <cade@anyscale.com>
cadedaniel
cadedaniel commented Jan 7, 2023
View reviewed changes
pytest.ini Show resolved Hide resolved
This was referenced Jan 7, 2023
Closed
Open
cadedaniel
cadedaniel commented Jan 7, 2023
View reviewed changes
python/ray/_private/runtime_env/context.py
@@ -66,7 +66,7 @@ def exec_worker(self, passthrough_args: List[str], language: Language):
else:
executable = "exec "

passthrough_args = [s.replace(" ", "\ ") for s in passthrough_args]
passthrough_args = [s.replace(" ", r"\ ") for s in passthrough_args]
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO(cade) make sure this passes tests, unclear what the purpose of this line is

Copy link
Member Author

Choose a reason for hiding this comment

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

@rkooo567 do you know who has context on this line? I don't know enough about java args to say whether the fix is good

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe @architkulkarni

@cadedaniel
Enabling default behavior for tests that verify warnings
5d67e6b 
Signed-off-by: Cade Daniel <cade@anyscale.com>
@cadedaniel cadedaniel force-pushed the fix-newer-python-syntax-errors branch from 84678ee to 1d4ca4a Compare January 8, 2023 03:26
@cadedaniel cadedaniel force-pushed the fix-newer-python-syntax-errors branch from 1d4ca4a to e4b9651 Compare January 8, 2023 03:32
@cadedaniel cadedaniel changed the title [Draft] [Fail on pytest warnings 1/n] Marking strings with invalid escape sequences as raw strings [Fail on pytest warnings 1/n] Marking strings with invalid escape sequences as raw strings Jan 9, 2023
@cadedaniel cadedaniel marked this pull request as ready for review January 9, 2023 06:02
@cadedaniel cadedaniel added the core Issues that should be addressed in Ray Core label Jan 9, 2023
cadedaniel
cadedaniel commented Jan 9, 2023
View reviewed changes
pytest.ini Outdated Show resolved Hide resolved
rkooo567
rkooo567 approved these changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

nice PR!

pytest.ini Outdated Show resolved Hide resolved
@rkooo567
Copy link
Contributor

(make sure to get approval from all stakeholders... I assume that's the most challenging part to merge the PR)

clarkzinzow
clarkzinzow approved these changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

Approving as Datasets codeowner! I double-checked and raw docstrings shouldn't result in different rendering of the docstrings.

Btw, another option is to escape the escapes in non-raw strings, but I think that raw strings are a better solution.

krfricke
krfricke approved these changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

LGTM, one question

rllib/algorithms/marwil/marwil.py Show resolved Hide resolved
@rkooo567
Copy link
Contributor

@cadedaniel can you double check the failure is not related to this PR? If so, I can merge it

@rkooo567 rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 11, 2023
@cadedaniel
Copy link
Member Author

@cadedaniel can you double check the failure is not related to this PR? If so, I can merge it

The failure is unrelated; I have #30388 but not the revert PR #31495. Let's merge.

@cadedaniel cadedaniel removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 11, 2023
@scv119 scv119 merged commit e54ff46 into ray-project:master Jan 11, 2023
@cadedaniel cadedaniel deleted the fix-newer-python-syntax-errors branch January 11, 2023 19:05
@cadedaniel
Copy link
Member Author

cadedaniel commented Jan 11, 2023 •
edited
Loading

This breaks the tests test_annotations.py::test_deprecated , test_runtime_context.py::test_ids, and test_actor_group.py::test_actor_creation in master because I didn't have a recent change #31195. Fixing forward in #31603, if there are more breakages then we should revert.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krfricke krfricke krfricke approved these changes

@avnishn avnishn avnishn approved these changes

@rkooo567 rkooo567 rkooo567 approved these changes

@clarkzinzow clarkzinzow clarkzinzow approved these changes

@scv119 scv119 scv119 approved these changes

@sven1977 sven1977 Awaiting requested review from sven1977 sven1977 is a code owner

@gjoliver gjoliver Awaiting requested review from gjoliver

@ArturNiederfahrenhorst ArturNiederfahrenhorst Awaiting requested review from ArturNiederfahrenhorst

@smorad smorad Awaiting requested review from smorad

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@kouroshHakha kouroshHakha Awaiting requested review from kouroshHakha

@ericl ericl Awaiting requested review from ericl

@jjyao jjyao Awaiting requested review from jjyao

@jianoaix jianoaix Awaiting requested review from jianoaix

@c21 c21 Awaiting requested review from c21

Assignees

@scv119 scv119

@rkooo567 rkooo567

Labels
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.
6 participants
@cadedaniel @rkooo567 @scv119 @clarkzinzow @krfricke @avnishn