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

[Data] [1/n] Fix dependency related Ray Data tests for Python 3.12 #46545

Closed
wants to merge 13 commits into from
Closed

[Data] [1/n] Fix dependency related Ray Data tests for Python 3.12 #46545

wants to merge 13 commits into from

Conversation

scottjlee
Copy link
Contributor

@scottjlee scottjlee commented Jul 10, 2024 •
edited
Loading

Why are these changes needed?

Fixes the following tests:

  • python/ray/data:test_mars

    • Fix: mark as manual, since mars library doesn't support Python 3.11+

  • linux://python/ray/data:test_formats

    • Fix: pin tensorflow-datasets==4.9.6

  • linux://python/ray/data:test_execution_optimizer

  • linux://doc/source/data/examples:stablediffusion_batch_prediction

    • Fix was attempted by pinning torch version installed in notebook from torch<2 to torch==2.3.0, but this results in an OOM/timeout on the CI docs test for an unknown reason.

  • linux://python/ray/data:test_tf

    • test_tf.py::TestToTF::test_element_spec_shape_with_ragged_tensors
    • test_tf.py::TestToTF::test_training: Still broken due to Keras 2 -> 3 upgrade breaking changes, forced in the TF 2.16.1 upgrade. We will need to file an issue with the Keras team to further investigate and resolve.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • 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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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 :(

@scottjlee
fixes
fd85e66 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee scottjlee added the go add ONLY when ready to merge, run all tests label Jul 10, 2024
can-anyscale
can-anyscale reviewed Jul 10, 2024
View reviewed changes
Copy link
Collaborator

@can-anyscale can-anyscale left a comment

Choose a reason for hiding this comment

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

fixing a lot of tests w00h00

python/requirements/ml/data-test-requirements.txt
# 4.9.6 fixes the following error with Python 3.12:
# `RecursionError: maximum recursion depth exceeded`
# See: https://github.com/tensorflow/datasets/issues/4666#issuecomment-2149200103
tensorflow-datasets==4.9.6
Copy link
Collaborator

Choose a reason for hiding this comment

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

w00t, you would need to update here too https://github.com/ray-project/ray/blob/ray-py312/python/requirements_compiled.txt#L1894 otherwise pip will complain during installation

@can-anyscale
Copy link
Collaborator

might need to update https://github.com/ray-project/ray/blob/ray-py312/python/requirements_compiled.txt#L1894 as well

@scottjlee
Copy link
Contributor Author

might need to update https://github.com/ray-project/ray/blob/ray-py312/python/requirements_compiled.txt#L1894 as well

thanks, i was hoping to use the output generated from build: pip-compile dependencies. or is it acceptable to just directly modify the corresponding line in requirements_compiled.txt? i was thinking that this may potentially impact other dependencies, so wanted to be sure.

@can-anyscale
Copy link
Collaborator

ah got you, you're doing it right, forget what i was saying

scottjlee and others added 6 commits July 10, 2024 16:00
@scottjlee
pin etils==1.9.1
86d3b90 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee
update requirements_compiled.txt from BK
8e0b07d 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee
Merge branch 'ray-py312' into 0710-py312-datatests
82bb46d 
Signed-off-by: Scott Lee <[email protected]>
@omatthew98
[py312] Fix test_tensor.py (ray-project#46669)
aefdf5f 
## Why are these changes needed?
To upgrade to py312 we need to upgrade to `pandas>=2.0.0`. This upgrade
introduced a breaking change in the syntax of some of our code / tests:
> Changed behavior in setting values with df.loc[:, foo] = bar or
df.iloc[:, foo] = bar, these now always attempt to set values inplace
before falling back to casting ([GH
45333](pandas-dev/pandas#45333))

As a result we needed to update these uses of loc to do direct
assignment.

## Related issue number

<!-- For example: "Closes ray-project#1234" -->

## Checks

- [ ] I've signed off every commit(by using the -s flag, i.e., `git
commit -s`) in this PR.
- [ ] 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 added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] 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 :(

Signed-off-by: Matthew Owen <[email protected]>
@scottjlee
Merge branch 'ray-py312' into 0710-py312-datatests
f1fc5b0
@scottjlee
update keras calls
b4b51dc 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee scottjlee marked this pull request as ready for review July 20, 2024 00:09
scottjlee
scottjlee commented Jul 20, 2024
View reviewed changes
python/requirements_compiled.txt
@@ -14,15 +14,15 @@ absl-py==1.4.0
# tensorflow-metadata
# tensorflow-probability
accelerate==0.28.0
# via -r /home/ubuntu/ray/ci/../python/requirements/ml/core-requirements.txt
# via -r /ray/ci/../python/requirements/ml/core-requirements.txt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, i used the output from here, but looks like it updated all these paths to/ray/ci? is this the correct way @can-anyscale ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hi yes this is ok; the /home/ubuntu was because i generated this file from my local machine and not using the one from CI

can-anyscale
can-anyscale approved these changes Jul 22, 2024
View reviewed changes
Copy link
Collaborator

@can-anyscale can-anyscale left a comment

Choose a reason for hiding this comment

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

omatthew98
omatthew98 approved these changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@omatthew98 omatthew98 left a comment

Choose a reason for hiding this comment

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

lgtm from the data side

@scottjlee
Copy link
Contributor Author

Closing in favor of #46730

@scottjlee scottjlee closed this Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omatthew98 omatthew98 omatthew98 approved these changes

@can-anyscale can-anyscale can-anyscale approved these changes

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

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

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

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

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

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

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

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

@stephanie-wang stephanie-wang Awaiting requested review from stephanie-wang stephanie-wang is a code owner

Assignees

@omatthew98 omatthew98

@can-anyscale can-anyscale

Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@scottjlee @can-anyscale @omatthew98