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

[Serve] warn when serve.start() with different options #21562

Merged
merged 5 commits into from
Jan 21, 2022
Merged

[Serve] warn when serve.start() with different options #21562

merged 5 commits into from
Jan 21, 2022

Conversation

jwyyy
Copy link
Contributor

@jwyyy jwyyy commented Jan 12, 2022 •
edited
Loading

Why are these changes needed?

This PR implements a short-term simple solution to #19868.

Related issue number

(partially) Closes #19868.

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 :(
      (Will add tests later once the implementation plan is finalized.)

@simon-mo simon-mo self-assigned this Jan 12, 2022
@jwyyy
Copy link
Contributor Author

jwyyy commented Jan 12, 2022

All failed client tests were successful locally (macOS Monterey 12.1).

simon-mo
simon-mo approved these changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@simon-mo simon-mo left a comment

Choose a reason for hiding this comment

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

Great work! Left few comments for cleaner error messages

python/ray/serve/api.py Outdated Show resolved Hide resolved
python/ray/serve/api.py Outdated Show resolved Hide resolved
python/ray/serve/api.py Outdated Show resolved Hide resolved
@simon-mo simon-mo added the @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. label Jan 14, 2022
jwyyy
jwyyy commented Jan 15, 2022
View reviewed changes
python/ray/serve/api.py
different_fields = []
all_http_option_fields = new_http_options.__dict__
for field in all_http_option_fields:
if getattr(new_http_options, field) != getattr(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we don't know what middlewares are used, we do a shallow check for each item in the list assuming that the ordering cannot be changed.

@jwyyy
Copy link
Contributor Author

jwyyy commented Jan 15, 2022 •
edited
Loading

Hi @simon-mo, thank you for your review! I updated this PR. (I will make another PR to add the update approach.)

Please let me know your feedback and comments when you have time to review it again. Thanks!

(Failed mac tests were successful locally. Other tests were successful in the previous commit.)

@jwyyy jwyyy requested a review from simon-mo January 18, 2022 19:05
@jwyyy
Copy link
Contributor Author

jwyyy commented Jan 21, 2022 •
edited
Loading

Hi @simon-mo, just want to follow up 😄 Do you have any comments and suggestions on the latest commit?

@simon-mo
Copy link
Contributor

Sorry if I should have point this out easier. Would you mind add a test in test_standalone?

@jwyyy
Copy link
Contributor Author

jwyyy commented Jan 21, 2022 •
edited
Loading

Sorry if I should have point this out easier. Would you mind add a test in test_standalone?

@simon-mo Thanks for your suggestion! Will add a test soon 😄

@jwyyy
Copy link
Contributor Author

jwyyy commented Jan 21, 2022 •
edited
Loading

Hi @simon-mo, I just added a warning test. Please let me know if I missed anything when you have time to take a look. Thanks a lot! (Tests passed locally.)

simon-mo
simon-mo approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@simon-mo simon-mo 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! Thank you

@jwyyy
Copy link
Contributor Author

jwyyy commented Jan 21, 2022

@simon-mo Thank you for your review!

@simon-mo
Copy link
Contributor

TypeError: 'type' object is not iterable
 

If you can merge master and push again that would be great! There was recently a dependency outage and master branch fixed it

@jwyyy
Copy link
Contributor Author

jwyyy commented Jan 21, 2022

If you can merge master and push again that would be great! There was recently a dependency outage and master branch fixed it

Hi @simon-mo, I merged the master branch. It seems the builder were okay. But some tests get stuck and have been running for hours. Could you take a look? Or I can simply make an empty commit and re-trigger the check. Thanks!

@simon-mo simon-mo merged commit 216c4bf into ray-project:master Jan 21, 2022
@simon-mo
Copy link
Contributor

ah that's because our mac builds were lagging. since windows and linux both passed, i went ahead and merged the PR.

@jwyyy
Copy link
Contributor Author

jwyyy commented Jan 21, 2022

ah that's because our mac builds were lagging. since windows and linux both passed, i went ahead and merged the PR.

Thank you for the check! I will go ahead and work on the update implementation. Thanks again for your review!

@jwyyy jwyyy deleted the serve_start_diff_option branch March 21, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simon-mo simon-mo simon-mo approved these changes

Assignees

@simon-mo simon-mo

Labels
@external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Serve] Calling serve.start(detached=True) with different options should error
2 participants
@jwyyy @simon-mo