Skip to content

Conversation

@rickyyx
Copy link
Member

@rickyyx rickyyx commented Nov 29, 2023

Why are these changes needed?

Closes #41338

The hypothesis is that we are importing some additional things when initializing the python workers, and changing the import to lazy seems to fix the issue.

See https://buildkite.com/ray-project/release/builds/2492#018c1923-c5d0-4e09-a67e-b45cf2c3b553

Master: tasks_per_seconds = 430
This PR: tasks_per_seconds = 530

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 :(

Signed-off-by: rickyyx <[email protected]>
@rickyyx rickyyx changed the title try lazy [core] Lazy import of pydantics Nov 29, 2023
@rickyyx
Copy link
Member Author

rickyyx commented Nov 29, 2023

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.

Q: consider adding a unit test to avoid regression? (i.e., import serialization_addon.py and make sure libs are not imported

@rickyyx
Copy link
Member Author

rickyyx commented Nov 29, 2023

Q: consider adding a unit test to avoid regression? (i.e., import serialization_addon.py and make sure libs are not imported

Sure - but i think what's more important is how we prevent this from happening again. Don't we have tests that track worker start-up time?

@rickyyx rickyyx closed this Nov 29, 2023
@rickyyx rickyyx reopened this Nov 29, 2023
Signed-off-by: rickyyx <[email protected]>

assert "ray" in sys.modules
for x in blocked_deps:
assert x not in sys.modules
Copy link
Member Author

Choose a reason for hiding this comment

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

test it failed the test on master.

@rkooo567 rkooo567 merged commit cef4c1f into ray-project:master Nov 30, 2023
fishbone added a commit that referenced this pull request Dec 1, 2023
rickyyx pushed a commit that referenced this pull request Dec 1, 2023
rickyyx added a commit that referenced this pull request Dec 1, 2023
rickyyx added a commit that referenced this pull request Dec 4, 2023
fishbone added a commit that referenced this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ci][core] Perf regression on tasks_per_second, pgs_per_second

3 participants