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.

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

Upgrade py-spy to 0.4.0 for py312 support #48978

Merged
merged 5 commits into from
Dec 4, 2024

Conversation

alanwguo
Copy link
Contributor

Why are these changes needed?

py-spy 0.4.0 has support for python 3.12.

Related issue number

Fixes #47388

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: Alan Guo <aguo@anyscale.com>
@@ -1519,7 +1519,7 @@ py-cpuinfo==9.0.0
# via deepspeed
py-partiql-parser==0.5.0
# via moto
py-spy==0.3.14
py-spy==0.4.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, you can upgrade this in python/requirements.txt files i think

Copy link
Collaborator

Choose a reason for hiding this comment

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

Signed-off-by: Alan Guo <aguo@anyscale.com>
Signed-off-by: Alan Guo <aguo@anyscale.com>
@@ -1519,7 +1519,7 @@ py-cpuinfo==9.0.0
# via deepspeed
py-partiql-parser==0.5.0
# via moto
py-spy==0.3.14
py-spy==0.4.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

this constraint file is used in py39 test environments, and I think this can lead to failures. I think we need to have separate requirement constraint files for different python versions in the long run. in the short run, maybe py-spy can be treated as a special case.

Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

  • the changes in python/setup.py and python/requirements.txt files are okay
  • the constraint file update probably has issues.

how is py-spy being tested, and are they tested on different python versions?

@alanwguo
Copy link
Contributor Author

alanwguo commented Dec 2, 2024

In my manual testing, the wheels still seem to have v0.3.14. Not sure what I need to do differently here.

There are automated tests in test_reporter.py which runs the test_get_task_traceback_running_task test.
I don't know the matrix of environments (if any) it runs on.

@aslonnie aslonnie self-requested a review December 2, 2024 21:35
@aslonnie
Copy link
Collaborator

aslonnie commented Dec 2, 2024

In my manual testing, the wheels still seem to have v0.3.14. Not sure what I need to do differently here.

could you tell me how you did the manual testing?

@alanwguo
Copy link
Contributor Author

alanwguo commented Dec 2, 2024

could you tell me how you did the manual testing?

I downloaded the wheel and launched a Anyscale workspace with that wheel installed.
specifically, I uploaded the wheel to S3 and used Anyscale container images to install the wheel from S3.

@richardliaw
Copy link
Contributor

richardliaw commented Dec 3, 2024

(looks good from my side for setup.py)

@@ -1519,7 +1519,7 @@ py-cpuinfo==9.0.0
# via deepspeed
py-partiql-parser==0.5.0
# via moto
py-spy==0.3.14
py-spy==0.4.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Signed-off-by: Alan Guo <aguo@anyscale.com>
@alanwguo alanwguo added the go add ONLY when ready to merge, run all tests label Dec 3, 2024
@alanwguo
Copy link
Contributor Author

alanwguo commented Dec 3, 2024

do we currently run all tests on all python versions? Is there a way to add configure a test file to run on python 3.12?
the test_reporter.py file.

@can-anyscale
Copy link
Collaborator

@alanwguo you can add the continuous-build tag into the PR, update branch again to kick off tests and it will run python 3.12 tests

@can-anyscale
Copy link
Collaborator

Signed-off-by: Alan Guo <aguo@anyscale.com>
@@ -1519,7 +1519,7 @@ py-cpuinfo==9.0.0
# via deepspeed
py-partiql-parser==0.5.0
# via moto
py-spy==0.3.14
py-spy==0.4.0 ; python_version < "3.12"
Copy link
Collaborator

Choose a reason for hiding this comment

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

so we just upgrade py-spy to 0.4.0, like for images of all python versions now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is reasonable behavior:
For our images, we use 0.4.0 out of the box.

For users installing ray themselves, we won't enforce 0.4.0 unless python 3.12 is used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

wonder why we did not upgrade py-spy earlier.. like was there something blocking us to do so? seems like a simple change based on this PR..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wasn't aware of 0.4.0's release. Which was released this november. I just saw someone comment that 0.4.0 had python 3.12 support so I updated after seeing that.

@can-anyscale can-anyscale merged commit 73a4c42 into ray-project:master Dec 4, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous-build go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard | Core?] Py-Spy and Python 3.12 - Lack of support
4 participants