Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Register all models after training, not only Segmentation models. #455

Merged
merged 28 commits into from
May 12, 2021

Conversation

Shruthi42
Copy link
Contributor

@Shruthi42 Shruthi42 commented May 7, 2021

This PR changes the codepath so all models trained on AzureML are registered. The codepath previously allowed only segmentation models (subclasses of SegmentationModelBase) to be registered. Models are registered after a training run or if the only_register_model flag is set. Models may be legacy InnerEye config-based models or may be defined using the LightningContainer class.

The PR also removes the AzureRunner conda environment. The full InnerEye conda environment is needed to submit a training job to AzureML.

It splits the TrainHelloWorldAndHelloContainer job in the PR build into two jobs, TrainHelloWorld and TrainHelloContainer. It adds a pytest marker after_training_hello_container for tests that can be run after training is finished in the TrainHelloContainer job.

This will solve the issue of model registration in #377 and #398.

InnerEye/ML/model_inference_config.py Outdated Show resolved Hide resolved
InnerEye/ML/run_ml.py Outdated Show resolved Hide resolved
InnerEye/ML/run_ml.py Show resolved Hide resolved
InnerEye/ML/run_ml.py Outdated Show resolved Hide resolved
InnerEye/ML/runner.py Show resolved Hide resolved
azure-pipelines/build-pr.yml Outdated Show resolved Hide resolved
# the current run is a single one. See the documentation of ModelProcessing for more details.
self.run_inference(self.checkpoint_handler, ModelProcessing.DEFAULT)

if self.container.generate_report:
Copy link
Contributor

Choose a reason for hiding this comment

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

[Not this PR] in the future we could allow users to define a generate report function for all containers i guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have one for general lightning containers actually, it is called separately in the else part of this if-else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened an issue to make the report generation codepath more consistent: #457.

ant0nsc
ant0nsc previously approved these changes May 12, 2021
InnerEye/ML/model_inference_config.py Show resolved Hide resolved
InnerEye/ML/run_ml.py Outdated Show resolved Hide resolved
InnerEye/ML/run_ml.py Show resolved Hide resolved
InnerEye/ML/run_ml.py Show resolved Hide resolved
melanibe
melanibe previously approved these changes May 12, 2021
Copy link
Contributor

@melanibe melanibe left a comment

Choose a reason for hiding this comment

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

Nice ! 🚀

@Shruthi42 Shruthi42 dismissed stale reviews from melanibe and ant0nsc via 8d377c8 May 12, 2021 10:22
@Shruthi42 Shruthi42 merged commit aa09b9d into main May 12, 2021
@Shruthi42 Shruthi42 deleted the shbannur/register-all-models branch May 12, 2021 14:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants