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

Clean up deployment suffixes between pipeline build() calls #23984

Merged
merged 10 commits into from
Apr 19, 2022

Conversation

jiaodong
Copy link
Member

@jiaodong jiaodong commented Apr 18, 2022

Why are these changes needed?

We generate new suffix to differentiate multiple instantiation of the same name with _1 _2 -- within same serve instance. For CLI it uses new serve instance upon every call so there's no issue, but for jupyter notebook equivalent this could be a bit annoying since our deployment name generator is at class level.

This PR makes name generation at instance level, and adds a simple cleanup / reset upon calling build() so we start with clean state for dag traversal when it's used in context manager.

Related issue number

Checks

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

Copy link
Contributor

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

@jiaodong it seems to me that the DeploymentNameGenerator should only be relevant for a single DAG traversal, right? Shouldn't it just be scoped to that traversal instead of globally scoped? Then we don't need to worry about resetting at all.

@jiaodong
Copy link
Member Author

That's indeed simpler. I was trying to avoid collision if user re-uses the same class multiple invocations within the same serve instance so it should generate new suffix, but i think in current @serve.deployment implementation we're also overwriting existing deployment with same class name so it's not introducing a new behavior. Let me make some changes.

@edoakes edoakes added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Apr 18, 2022
@jiaodong jiaodong changed the title Fix unique name Clean up deployment suffixes between pipeline build() calls Apr 19, 2022
@jiaodong
Copy link
Member Author

Failed tests are related to tensorflow, tutorial_rllib, tutorial_tensorflow and documentation link checks, not related to this PR. Previous commit b5f9190 also had it all green.

@jiaodong jiaodong added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. flaky-test and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Apr 19, 2022
@edoakes
Copy link
Contributor

edoakes commented Apr 19, 2022

@jiaodong please update PR description.

@edoakes edoakes added @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. and removed tests-ok The tagger certifies test failures are unrelated and assumes personal liability. flaky-test labels Apr 19, 2022
@simon-mo
Copy link
Contributor

Discussed offline. One solution could be just don't make DeploymentName generator a global singleton, rather just passed it into the transform_ray_dag as an instance.

Copy link
Contributor

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

Looks great! I think we can further simplify by removing the locking logic now that it isn't a global singleton :)

python/ray/serve/pipeline/generate.py Outdated Show resolved Hide resolved
python/ray/serve/pipeline/generate.py Outdated Show resolved Hide resolved
Copy link
Contributor

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

LGTM

@simon-mo simon-mo merged commit 5ba29f0 into ray-project:master Apr 19, 2022
@jiaodong jiaodong added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants