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

Configurable startup time #3262

Merged
merged 24 commits into from
Aug 12, 2024
Merged

Conversation

Isalia20
Copy link
Contributor

@Isalia20 Isalia20 commented Jul 22, 2024

Description

Adds feature from #3261 and from #2452

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Feature/Issue validation/testing

  • Test for long startup while having small response timeout. Test can be found in test/pytest/test_startup_timeout.py

================================================================================ test session starts =================================================================================
platform linux -- Python 3.10.14, pytest-7.3.1, pluggy-1.0.0
rootdir: /home/isalia/Desktop/torchserve
plugins: mock-3.14.0, timeout-2.3.1, cov-4.1.0, typeguard-4.3.0
collected 1 item

test/pytest/test_startup_timeout.py . [100%]

================================================================================= 1 passed in 15.02s =================================================================================

Checklist:

  • Did you have fun?
  • Have you added tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

@Isalia20 Isalia20 marked this pull request as ready for review July 26, 2024 21:02
@agunapal agunapal requested a review from mreso August 2, 2024 22:50
Copy link
Collaborator

@mreso mreso left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Please see my comment regarding testing. Otherwise this look good to go after adjusting the test.

test/pytest/test_startup_timeout.py Show resolved Hide resolved
test/pytest/test_startup_timeout.py Show resolved Hide resolved
@Isalia20
Copy link
Contributor Author

Isalia20 commented Aug 7, 2024

I see some checks are failing but I'm not sure what the issue really is, to fix it

@mreso
Copy link
Collaborator

mreso commented Aug 7, 2024

I see some checks are failing but I'm not sure what the issue really is, to fix it

Yeah, these regression tests can be a pain. Taking a look

@Isalia20
Copy link
Contributor Author

Isalia20 commented Aug 7, 2024

One of the tests was failing due to startup_timeout not being cast to int in test/pytest/test_gRPC_management_apis.py. Added it to the list of params that should be casted to int

@Isalia20
Copy link
Contributor Author

Seems like tests have passed(required ones). Docker ones are failing due to:
0.811 fatal: Remote branch configurable-startup-time not found in upstream origin
which could be because it can't clone from my branch 🤔

@mreso
Copy link
Collaborator

mreso commented Aug 12, 2024

Hi @Isalia20 yes, that should be the docker build script which only supports pytorch/serve as remote. We can proceed with merging this PR. Thanks again for your contribution!

@mreso mreso self-requested a review August 12, 2024 22:46
Copy link
Collaborator

@mreso mreso left a comment

Choose a reason for hiding this comment

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

LGTM now

@mreso mreso added this pull request to the merge queue Aug 12, 2024
Merged via the queue into pytorch:master with commit ef196c0 Aug 12, 2024
9 of 12 checks passed
@maaquib maaquib mentioned this pull request Sep 9, 2024
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.

2 participants