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] Apply request_timeout_s from Serve config to the cluster #37884

Merged
merged 6 commits into from
Jul 28, 2023
Merged

[Serve] Apply request_timeout_s from Serve config to the cluster #37884

merged 6 commits into from
Jul 28, 2023

Conversation

shrekris-anyscale
Copy link
Contributor

Why are these changes needed?

Currently, the Serve config's request_timeout_s field is ignored. This change makes Serve pass in request_timeout_s to the HTTP Proxies when starting up, so the field is respected.

Related issue number

Closes #37882.

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
      • This change adds a unit test to test_request_timeout.py.

@shrekris-anyscale
Add test for timeout
412f505 
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
@shrekris-anyscale
Update docs
546392d 
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
GeneDer
GeneDer reviewed Jul 28, 2023
View reviewed changes
Copy link
Contributor

@GeneDer GeneDer left a comment

Choose a reason for hiding this comment

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

Thanks Shreyas for quick turnaround! Really just wondering if we should also change line 193 for deployments api as well

dashboard/modules/serve/serve_agent.py Outdated
except ValidationError as e:
return Response(
status=400,
text=repr(e),
)

config_http_options = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just do config_http_options = config.http_options.dict() so everything will be included

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this will prevent similar issues from happening in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, that makes the code a lot simpler too!

dashboard/modules/serve/serve_agent.py Outdated
# Check HTTP location
location_conflict = self.check_http_options(
# Handle location conflict separately since proxy_location isn't stored
# inside config.http_options
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocker: If we just store line 271 as new_http_configs or something, then we can check all of them in the same loop :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, I stored all the configs as full_http_options and checked the Serve application's options against that dictionary.

sihanwang41
sihanwang41 reviewed Jul 28, 2023
View reviewed changes
dashboard/modules/serve/serve_agent.py Outdated
"host": config.http_options.host,
"port": config.http_options.port,
"root_path": config.http_options.root_path,
"request_timeout_s": config.http_options.request_timeout_s,
Copy link
Contributor

Choose a reason for hiding this comment

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

propose to http_request_timeout_s.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a bugfix, no API changes

edoakes
edoakes reviewed Jul 28, 2023
View reviewed changes
Copy link
Collaborator

@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.

One small comment echoing Gene then LGTM

dashboard/modules/serve/serve_agent.py Outdated
except ValidationError as e:
return Response(
status=400,
text=repr(e),
)

config_http_options = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this will prevent similar issues from happening in the future

dashboard/modules/serve/serve_agent.py Outdated
"host": config.http_options.host,
"port": config.http_options.port,
"root_path": config.http_options.root_path,
"request_timeout_s": config.http_options.request_timeout_s,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a bugfix, no API changes

@shrekris-anyscale
Handle http options more generally
1097cee 
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
@shrekris-anyscale
Use .items() when unpacking dictionary
9c973fe 
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
@shrekris-anyscale
Check for None explicitly
1ab5000 
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
@shrekris-anyscale
Merge branch 'master' of github.com:ray-project/ray into set_timeout_…
a706130 
…config
GeneDer
GeneDer approved these changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@GeneDer GeneDer left a comment

Choose a reason for hiding this comment

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

:shipit:

@edoakes edoakes merged commit b2d81a7 into ray-project:master Jul 28, 2023
@shrekris-anyscale shrekris-anyscale mentioned this pull request Jul 28, 2023
Merged
8 tasks
shrekris-anyscale added a commit to shrekris-anyscale/ray that referenced this pull request Jul 28, 2023
@shrekris-anyscale
[Serve] Apply request_timeout_s from Serve config to the cluster (r…
8eb8c33 
…ay-project#37884)

Currently, the Serve config's `request_timeout_s` field is ignored. This change makes Serve pass in `request_timeout_s` to the HTTP Proxies when starting up, so the field is respected.

Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
rickyyx pushed a commit that referenced this pull request Jul 29, 2023
@shrekris-anyscale
[Serve] Apply request_timeout_s from Serve config to the cluster (#…
92ad4ba 
…37884) (#37903)

Currently, the Serve config's `request_timeout_s` field is ignored. This change makes Serve pass in `request_timeout_s` to the HTTP Proxies when starting up, so the field is respected.

Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
NripeshN pushed a commit to NripeshN/ray that referenced this pull request Aug 15, 2023
@shrekris-anyscale @NripeshN
[Serve] Apply request_timeout_s from Serve config to the cluster (r…
ca8d359 
…ay-project#37884)

Currently, the Serve config's `request_timeout_s` field is ignored. This change makes Serve pass in `request_timeout_s` to the HTTP Proxies when starting up, so the field is respected.

Signed-off-by: NripeshN <nn2012@hw.ac.uk>
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
@shrekris-anyscale @arvind-chandra
[Serve] Apply request_timeout_s from Serve config to the cluster (r…
78ca863 
…ay-project#37884)

Currently, the Serve config's `request_timeout_s` field is ignored. This change makes Serve pass in `request_timeout_s` to the HTTP Proxies when starting up, so the field is respected.

Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
@shrekris-anyscale
[Serve] Apply request_timeout_s from Serve config to the cluster (r…
862753c 
…ay-project#37884)

Currently, the Serve config's `request_timeout_s` field is ignored. This change makes Serve pass in `request_timeout_s` to the HTTP Proxies when starting up, so the field is respected.

Signed-off-by: Victor <vctr.y.m@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sihanwang41 sihanwang41 sihanwang41 approved these changes

@GeneDer GeneDer GeneDer approved these changes

@edoakes edoakes edoakes approved these changes

@zcin zcin zcin approved these changes

@architkulkarni architkulkarni Awaiting requested review from architkulkarni

Assignees

@GeneDer GeneDer

@edoakes edoakes

@akshay-anyscale akshay-anyscale

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Serve] request_timeout_s is ignored in Serve config
6 participants
@shrekris-anyscale @sihanwang41 @GeneDer @edoakes @zcin @akshay-anyscale