feat: Load new model version should not reload loaded existing model version(s) #388
Conversation
… model version files
kthui
force-pushed
the
jacky-load-new-model-version
branch
from
bf815d6
to
3d2f5c3
Compare
| // model files | ||
| mtime.second = std::max(mtime.second, GetModifiedTime(full_path)); | ||
| model_timestamps_.clear(); | ||
| return; |
There was a problem hiding this comment.
raise exception and try catch?
There was a problem hiding this comment.
Did a quick test on L0_lifecycle, the test currently expects any error related to retrieving timestamp to remain hidden and simply set the failed timestamp to 0. To translate it into this ModelTimestamp object, setting timestamp to 0 is the same as initializing the object using the default constructor, which is leaving the model_timestamps_ object empty. Thus, if an exception is raised here, the caller catching the exception will simply re-initialize ModelTimestamp object using the default constructor (to remain aligned with the current logic of setting the timestamp to 0). In this case, raising an exception is redundant and can be handled by the constructor in one or two lines, or with a helper function specifically for keeping everything empty.
Another approach is we can change the test cases to accept the behavior change that failure to determine timestamp will fail the model load.
There was a problem hiding this comment.
Retaining existing error behavior for the cherry-pick and enhancing in a follow-up sounds fine to me. If we really intend to do it - please put in a // DLIS-XXXX: blah blah so it doesn't get lost.
There was a problem hiding this comment.
Sure, added the comment: Comment on should raise an exception when failed to create timestamp
There was a problem hiding this comment.
Can be addressed as follow-up, but I do think an exception should be raised, and it is fine to have the caller catch the exception and then initialize a timestamp with default constructor (if not failing). This is from the stand point where if the user initializes a timestamp with a path, the user is expecting the returned timestamp to carry correct timestamp w.r.t. the given path, not 0. And thus an error should be raised programmatically and it is up to the user to decide how to handle the error.
…version(s) (#388) * Do not reload unmodified loaded model version * Track model directory timestamps more granularly to detect updates to model version files * Rename model config util config change require reload function * Re-organize ModelTimestamp() and throw exception * Revert "Re-organize ModelTimestamp() and throw exception" This reverts commit 41e57e5. * Break constructor into multiple functions * Comment on should raise an exception when failed to create timestamp
…version(s) (#388) * Do not reload unmodified loaded model version * Track model directory timestamps more granularly to detect updates to model version files * Rename model config util config change require reload function * Re-organize ModelTimestamp() and throw exception * Revert "Re-organize ModelTimestamp() and throw exception" This reverts commit 41e57e5. * Break constructor into multiple functions * Comment on should raise an exception when failed to create timestamp
…version(s) (#388) (#390) * Do not reload unmodified loaded model version * Track model directory timestamps more granularly to detect updates to model version files * Rename model config util config change require reload function * Re-organize ModelTimestamp() and throw exception * Revert "Re-organize ModelTimestamp() and throw exception" This reverts commit 41e57e5. * Break constructor into multiple functions * Comment on should raise an exception when failed to create timestamp
What does the PR do?
It is requested that any model version(s) already loaded should not be reloaded upon adding and loading new version(s), if the already loaded version(s) are not modified. This is a small optimization to the model load/unload logic to avoid reloading unchanged model version(s) unload a load request.
Checklist
<commit_type>: <Title>Commit Type:
Check the conventional commit type
box here and add the label to the github PR.
Related PRs:
triton-inference-server/server#7527
Where should the reviewer start?
Start with the changes to model_lifecycle to see why the small twist to the logic enables the previously reloaded model version to be updated, and then move towards model_config_utils and model_repository_manager to see how those changes support the decision to update vs reload.
Test plan:
New tests on not reloading already loaded model version upon loading other model versions is added. See the tests on server PR.
Caveats:
N/A
Background
Previously, the model (all versions) will always be reloaded if there is a change in the model directory beyond the model config. This will cause unnecessary reload of unmodified model version(s), which this change makes model directory change detection more granular and decides if a model version should be reloaded or updated based on the model file(s) of the version.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
N/A