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

Fix set model state if runtime is null #2928

Merged
merged 6 commits into from
Feb 8, 2024

Conversation

mreso
Copy link
Collaborator

@mreso mreso commented Feb 5, 2024

Description

This PR fixes an issue when a model has no runtime set in the snapshot and we load with a newer torchserve version which expects the field.

Fixes #(issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Feature/Issue validation/testing

Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

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?

@mreso mreso marked this pull request as ready for review February 6, 2024 21:56
@mreso mreso requested review from agunapal and lxning February 6, 2024 21:56
Copy link
Collaborator

@agunapal agunapal left a comment

Choose a reason for hiding this comment

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

How do we make sure that someone running 0.9.0 TS with snapshot upgrading to the latest will not run into an issue.
Can we test this manually and add logs here?

@mreso
Copy link
Collaborator Author

mreso commented Feb 7, 2024

@ankithagunapal the new test test_empty_runtime covers that scenario by removing the runtime entry from the snapshot, but sure lets do this once manually.

@mreso
Copy link
Collaborator Author

mreso commented Feb 8, 2024

How do we make sure that someone running 0.9.0 TS with snapshot upgrading to the latest will not run into an issue. Can we test this manually and add logs here?

Here is the log of the test:
https://gist.github.com/mreso/f3ba1b41489e8bb76856b295ecacfadf

Copy link
Collaborator

@agunapal agunapal left a comment

Choose a reason for hiding this comment

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

LGTM

@agunapal agunapal added this pull request to the merge queue Feb 8, 2024
Merged via the queue into master with commit 7c4559c Feb 8, 2024
13 checks passed
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