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

Add is best model to training servicer #229

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

thodkatz
Copy link
Collaborator

@thodkatz thodkatz commented Dec 13, 2024

It builds upon #228 .

Whenever we have a new best model, the IsBestModel stream will yield a response. The client can utilize this to perform certain actions e.g. ilastik to propagateDirty any predictions performed on previous models.

- Supported operations: start, resume, pause, shutdown
- pytorch-3dunet package is used as the framework to create the models
… failed

I caught an edge case, where events are blocked, because we have exited the training, and the tasks of the queue would remain unprocessed.
Creating and closing processes and threads can be quite time consuming
resulting to test timeouts if the tests performs a lot of actions.
Applying monkeypatch to a parent process won't propagated to a child process if the start method is spawn (macos) instead of fork (linux)
- To fix test on windows, convert label data to float64
The should stop callbacks are boolean, so we need to aggregate their return value. Previously the return value wasn't taken into account, and the callbacks were returning none
The enum is used for validation check before triggering one of them. Previously I was checking if the queue was alive, but that won't be enough, for example if you want to perform resume, while you are resumed, the queue is operational, but the action shouldn't be valid.
@thodkatz thodkatz force-pushed the add-is-best-model-to-training-servicer branch 3 times, most recently from b60fb5f to 90b4a5e Compare December 19, 2024 11:15
Move NamedInt and Tensor proto to a separate file so training proto can
use as well
- The inference servicer had a procedure to list the available devices.
  This is needed or the training servicer as well. So list devices is
  decoupled to be shared.
If the training is running or paused, the forward, will retain the state
after completion. But it requires to pause so we can release memory and
do the forward pass.
@thodkatz thodkatz force-pushed the add-is-best-model-to-training-servicer branch from 90b4a5e to 811ddd3 Compare December 20, 2024 22:04
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 62.19512% with 372 lines in your changes missing coverage. Please review.

Project coverage is 62.83%. Comparing base (5ea5d3a) to head (14f81af).

Files with missing lines Patch % Lines
tiktorch/trainer.py 42.53% 127 Missing ⚠️
tiktorch/proto/training_pb2_grpc.py 54.78% 52 Missing ⚠️
tiktorch/server/session/backend/supervisor.py 68.86% 52 Missing ⚠️
tiktorch/proto/training_pb2.py 27.02% 27 Missing ⚠️
tiktorch/proto/utils_pb2.py 30.00% 21 Missing ⚠️
tiktorch/proto/inference_pb2.py 20.00% 20 Missing ⚠️
tiktorch/server/session/backend/commands.py 78.40% 19 Missing ⚠️
tiktorch/server/session/backend/base.py 66.66% 17 Missing ⚠️
tiktorch/server/session/process.py 72.34% 13 Missing ⚠️
tiktorch/server/session/rpc_interface.py 71.42% 10 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
- Coverage   64.60%   62.83%   -1.78%     
==========================================
  Files          40       47       +7     
  Lines        2195     2876     +681     
==========================================
+ Hits         1418     1807     +389     
- Misses        777     1069     +292     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Since both inference and training servicers have common the concept of
id, the training session id was replaced with the model session one used
for inference. This model session protobuf interfaced moved to a
separate utils proto file.

The PredictRequest being common, can be leveraged for abstraction.
@thodkatz thodkatz force-pushed the add-is-best-model-to-training-servicer branch from 811ddd3 to b10d4f9 Compare December 21, 2024 10:52
- If the model was initial paused or running, save after completion
  retain the state, while temporarily pausing to perform the save.
- The export will pause the training if not paused before.
Whenever we have a new model, a stream will yield a response. The client
can utilize this to perform certain actions e.g. ilastik to
propagateDirty any predictions performed on previous models.
The response of the best model stream will return an id. The id is
increased by one, each time we have a new model. A client can identify
if an action has been performed by an outdated model based on the id. If
    the current is greater, then a new best model exists.
@thodkatz thodkatz force-pushed the add-is-best-model-to-training-servicer branch from b10d4f9 to 14f81af Compare December 21, 2024 11:15
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.

1 participant