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

cadedaniel · 2023-01-07T22:33:03Z

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>

pytest.ini

cadedaniel · 2023-01-07T23:03:05Z

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]


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

@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

maybe @architkulkarni

Signed-off-by: Cade Daniel <cade@anyscale.com>

pytest.ini

rkooo567

nice PR!

pytest.ini

rkooo567 · 2023-01-10T14:14:50Z

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

clarkzinzow

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

LGTM, one question

rllib/algorithms/marwil/marwil.py

rkooo567 · 2023-01-11T08:45:09Z

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

cadedaniel · 2023-01-11T18:27:38Z

@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 · 2023-01-11T20:17:50Z

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.

…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>

Marking strings with invalid escape sequences as raw strings

b0cd0c7

Signed-off-by: Cade Daniel <cade@anyscale.com>

cadedaniel commented Jan 7, 2023

View reviewed changes

pytest.ini Show resolved Hide resolved

This was referenced Jan 7, 2023

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

Closed

[core] Fail OSS CI on pytest warnings #31479

Open

cadedaniel commented Jan 7, 2023

View reviewed changes

Enabling default behavior for tests that verify warnings

5d67e6b

Signed-off-by: Cade Daniel <cade@anyscale.com>

cadedaniel force-pushed the fix-newer-python-syntax-errors branch from 84678ee to 1d4ca4a Compare January 8, 2023 03:26

Missed enabling warning in test.

e4b9651

cadedaniel force-pushed the fix-newer-python-syntax-errors branch from 1d4ca4a to e4b9651 Compare January 8, 2023 03:32

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 marked this pull request as ready for review January 9, 2023 06:02

cadedaniel requested review from sven1977, gjoliver, avnishn, ArturNiederfahrenhorst, smorad, maxpumperla, kouroshHakha, krfricke, ericl, scv119, clarkzinzow, jjyao, jianoaix and c21 as code owners January 9, 2023 06:02

cadedaniel assigned rkooo567 and scv119 Jan 9, 2023

cadedaniel added the core Issues that should be addressed in Ray Core label Jan 9, 2023

cadedaniel commented Jan 9, 2023

View reviewed changes

pytest.ini Outdated Show resolved Hide resolved

rkooo567 approved these changes Jan 10, 2023

View reviewed changes

pytest.ini Outdated Show resolved Hide resolved

PR comments

8197d64

scv119 approved these changes Jan 10, 2023

View reviewed changes

clarkzinzow approved these changes Jan 10, 2023

View reviewed changes

avnishn approved these changes Jan 10, 2023

View reviewed changes

krfricke approved these changes Jan 10, 2023

View reviewed changes

rllib/algorithms/marwil/marwil.py Show resolved Hide resolved

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 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 merged commit e54ff46 into ray-project:master Jan 11, 2023

cadedaniel deleted the fix-newer-python-syntax-errors branch January 11, 2023 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cadedaniel commented Jan 7, 2023 •

edited

cadedaniel Jan 7, 2023

cadedaniel Jan 9, 2023

rkooo567 Jan 9, 2023

rkooo567 left a comment

rkooo567 commented Jan 10, 2023

clarkzinzow left a comment

krfricke left a comment

rkooo567 commented Jan 11, 2023

cadedaniel commented Jan 11, 2023

cadedaniel commented Jan 11, 2023 •

edited

	... 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

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

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

Conversation

cadedaniel commented Jan 7, 2023 • edited

Background

Pytest?

Example offenders

cadedaniel Jan 7, 2023

Choose a reason for hiding this comment

cadedaniel Jan 9, 2023

Choose a reason for hiding this comment

rkooo567 Jan 9, 2023

Choose a reason for hiding this comment

rkooo567 left a comment

Choose a reason for hiding this comment

rkooo567 commented Jan 10, 2023

clarkzinzow left a comment

Choose a reason for hiding this comment

krfricke left a comment

Choose a reason for hiding this comment

rkooo567 commented Jan 11, 2023

cadedaniel commented Jan 11, 2023

cadedaniel commented Jan 11, 2023 • edited

cadedaniel commented Jan 7, 2023 •

edited

cadedaniel commented Jan 11, 2023 •

edited