Updated Tuner #26
Updated Tuner #26
Conversation
Test Results 6 files 6 suites 37m 35s ⏱️ For more details on these failures, see this check. Results for commit 88b5eab. ♻️ This comment has been updated with latest results. |
|
Now tuning runs are nested together under common parent experiment and can be more easily compared in MLFLow. Parent tracker is only used to report hyperparameters that are currently the best in the whole study. CC: @tersekmatija |
luxonis_train/core/tuner.py
Outdated
| **tracker_params, | ||
| ) | ||
| if self.parent_tracker.is_mlflow: | ||
| run = self.parent_tracker.experiment["mlflow"].active_run() |
There was a problem hiding this comment.
Will parent run remain active long enough for this to not cause any issues?
There was a problem hiding this comment.
Yes, this run is active until the very end when whole study finishes. Basically every trial in the study is its own training run (like we would run luxonis_train train...). And every trial gets its own tracker. To nest them together we need one top-level tracker - parent tracker. In the backend mlflow has a stack of all active runs and when nest=True it binds it to the previous run in the stack. So this top level tracker is only used for binding all trials together and logging best hyperparameters in the whole study - for easy of use, to quickly get the results of the study.
luxonis_train/core/tuner.py
Outdated
| ) | ||
| if self.parent_tracker.is_mlflow: | ||
| run = self.parent_tracker.experiment["mlflow"].active_run() | ||
| self.parent_run_id = run.info.run_id |
There was a problem hiding this comment.
Is this used somewhere? What is this set to if MLFlow is not used?
There was a problem hiding this comment.
If MLFlow is not used then still this first tracker will report the best hyperparameters but nesting won't be done because it is not supported by e.g. Tensorboard. So every trial will have it's own tensorboard logs with no connection between them. That's why when tuning using MLFlow is recommended.
The whole part in this if block seems like it is not actually needed but if I remove it then nesting is not doen for some reason. Looking into it and I'll update you
There was a problem hiding this comment.
Ok sounds good. Not critical. I think looks good, maybe we just add a comment in code to note this is required for MLFlow to work correctly, then we can merge.
There was a problem hiding this comment.
I figured it out: The way tracker is written it creates the actual mlflow run once it is interacted with. And since in our case we need parent tracker to be created first (to then nest all children runs to that one) we need to kep this line in the code. I added a note to the code.
CC: @kozlov721 I think now we can merge
There was a problem hiding this comment.
Is this specific to "mlflow" then, or should this be present for other underlying trackers as well? @klemen1999
There was a problem hiding this comment.
It's the case for all trackers. LuxonisTracker is sctructured very similar to other default PL loggers (e.g. MLFlowLogger) where the actual objects are created when .experiment property is first called. This ensures that LuxonisTracker is nicely itegrated with PL but we have to be mindfull of that when using it like this. Although I don't think there will be many cases where one would create just a placeholder tracker, like we are doing for this specific case, so it shouldn't cause too many problems.
There was a problem hiding this comment.
So should we always call this instead of only on:
if self.parent_tracker.is_mlflow
There was a problem hiding this comment.
mlflow is the only tracker that this nesting applies to so that is why I'm doing it just for this.
There was a problem hiding this comment.
Sounds good, I think we can proceed with the merge in this case.
|
I've added graceful stop for |
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
…ead of bare tensors
…put_sources, which tells the LuxonisModel which loader sub-elements it wants to load, and LoaderConfig has an images_name parameter which identifies the image-like input among the sub-elements for compatibility with visualizers etc.
…ng use of images_name.
…to code structure requirements. Added missing tests for evaluation, export, and inference.
Co-authored-by: Martin Kozlovsky <[email protected]> Co-authored-by: Michal Sejak <[email protected]> Co-authored-by: GitHub Actions <[email protected]>
continue_existing_study: bool = Trueparameter to the config. If this is set to True (default) then it will use existing study with that name otherwise if name already exists and this is set to False it will throw an error.