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

Allow cross validation with 'bring your own' Lightning models (without ensemble building) #483

Merged
merged 10 commits into from
Jun 24, 2021
Merged

Allow cross validation with 'bring your own' Lightning models (without ensemble building) #483

merged 10 commits into from
Jun 24, 2021

Conversation

dumbledad
Copy link
Contributor

@dumbledad dumbledad commented Jun 8, 2021 •
edited
Loading

When complete, will be closing #476

  • Ensure that your PR is small, and implements one change.
  • Add unit tests for all functions that you introduced or modified.
  • Run PyCharm's code cleanup tools on your Python files. (I'm using VSCode)
  • Link the correct GitHub issue for tracking.
  • Update the Changelog file: Describe your change in terms of
    Added/Changed/Removed/... in the "Upcoming" section.
  • When merging your PR, replace the default merge message with a description of your PR,
    and if needed a motivation why that change was required.

@dumbledad dumbledad requested a review from ant0nsc June 11, 2021 10:53
ant0nsc
ant0nsc suggested changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@ant0nsc ant0nsc left a comment

Choose a reason for hiding this comment

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

Looking good so far, but please increase test coverage. At the moment, there is no clear proof in the tests that submitting an AML job with a container model and crossval would actually work.

@dumbledad
Copy link
Contributor Author

Looking good so far, but please increase test coverage. At the moment, there is no clear proof in the tests that submitting an AML job with a container model and crossval would actually work.

That's a good idea. I thought we had agreed to use unit tests that did not actually run an AML experiment so that the test did not incur a time penalty and a cost. But I think an end-to-end test with known outputs from a known model is a safer bet. I'll do that.

dumbledad added a commit that referenced this pull request Jun 15, 2021
@dumbledad
Expanding abstract method documentation
f7c1fc7 
As per #483 (comment)
dumbledad added a commit that referenced this pull request Jun 20, 2021
@dumbledad
Expanding abstract method documentation
f090c58 
As per #483 (comment)
@dumbledad dumbledad force-pushed the timregan/3814-cross-validation-lightning branch from 6b73793 to 87d0e47 Compare June 20, 2021 19:07
@dumbledad dumbledad marked this pull request as ready for review June 20, 2021 20:18
@dumbledad dumbledad enabled auto-merge (squash) June 21, 2021 07:36
ant0nsc
ant0nsc suggested changes Jun 21, 2021
View reviewed changes
@dumbledad dumbledad self-assigned this Jun 21, 2021
@dumbledad dumbledad force-pushed the timregan/3814-cross-validation-lightning branch 3 times, most recently from 9d97d43 to dba0588 Compare June 21, 2021 19:20
dumbledad pushed a commit that referenced this pull request Jun 21, 2021
This is a combination of 36 commits.
dba0588 
This is the 1st commit message:

remove exception on lightningcontainer as ensembles with cross-validation

This is the commit message #2:

Removing unnecessary lambda

This is the commit message #3:

Removing block for local run

This is the commit message #4:

Dealing with missing config for hyperdrive in LightningContainer CrossVal

This is the commit message #5:

Old typo in comment

This is the commit message #6:

Refactoring hyperdrive cross validation support into WorkflowParams

This is the commit message #7:

property not blocked fro cross validation

This is the commit message #8:

inconsistent blank rows

This is the commit message #9:

tidy up

This is the commit message #10:

Restoring block on offline segmentation cross validation

AzureML is working, but offline is not. How come? The AzureML experiment runs
'offline' on Azure so how does it pass?

Anyway, enabling block for now while writing test.

This is the commit message #11:

Commenting out blocks on offline cross validation for segmentation models

When I remove lines 290, 342, and 343 from run_ml.py,  i.e. when I remove the blocks on offline cross validation
for segmentation models, then I hit the error:
	'NoneType' object has no attribute 'number_of_cross_validation_splits'
because in line 305, in spawn_offline_cross_val_classification_child_runs:
	for i in range(self.innereye_config.number_of_cross_validation_splits):
self.innereye_config is None

This is the commit message #12:

Parking offline, since it will only work for 1 GPU anyway

This is the commit message #13:

unused param better as _

This is the commit message #14:

unit test for lightningcontainer get_hyperdrive_config

This is the commit message #15:

MyPy fix

This is the commit message #16:

Removing get_total_number_of_cros_validation_runs

Remnant of an old feature

This is the commit message #17:

flake8 fixes

This is the commit message #18:

Expanding abstract method documentation

As per #483 (comment)

This is the commit message #19:

Reverting, mypy errors

How can changing two abstract method's doc-strings cause mypy errors?

This is the commit message #20:

Removing unneeded Optional

This is the commit message #21:

We do need that property

But fixing typing via an exception feels all wrong

This is the commit message #22:

Extending abstract method documentation

This is the commit message #23:

Unit test for cross validation lightning changes to runner

This is the commit message #24:

Adding cross validation to HelloContainer

This is the commit message #25:

Correcting HalloContainer xval splits + comments

This is the commit message #26:

Unit test for xval work in HalloContainer

But test not passing MyPy yet, grrrrrrr

This is the commit message #27:

mypy fixes on unit test

This is the commit message #28:

updating changelog

This is the commit message #29:

testing val sets add up correctly

This is the commit message #30:

finishing documentation

This is the commit message #31:

adding comments

This is the commit message #32:

Refactoring HelloDataset

To remove clumsy init method parameters

This is the commit message #33:

homegrown -> sklearn.model_selection.KFold

This is the commit message #34:

Dropping notimplemented override

This is the commit message #35:

Restoring override in correct place

This is the commit message #36:

Refactoring get_parameter_search_hyperdrive_config

As per Anton's comment
@dumbledad
o# This is a combination of 7 commits.
6686a1b 
This is a combination of 36 commits.
This is the 1st commit message:

remove exception on lightningcontainer as ensembles with cross-validation

This is the commit message #2:

Removing unnecessary lambda

This is the commit message #3:

Removing block for local run

This is the commit message #4:

Dealing with missing config for hyperdrive in LightningContainer CrossVal

This is the commit message #5:

Old typo in comment

This is the commit message #6:

Refactoring hyperdrive cross validation support into WorkflowParams

This is the commit message #7:

property not blocked fro cross validation

This is the commit message #8:

inconsistent blank rows

This is the commit message #9:

tidy up

This is the commit message #10:

Restoring block on offline segmentation cross validation

AzureML is working, but offline is not. How come? The AzureML experiment runs
'offline' on Azure so how does it pass?

Anyway, enabling block for now while writing test.

This is the commit message #11:

Commenting out blocks on offline cross validation for segmentation models

When I remove lines 290, 342, and 343 from run_ml.py,  i.e. when I remove the blocks on offline cross validation
for segmentation models, then I hit the error:
	'NoneType' object has no attribute 'number_of_cross_validation_splits'
because in line 305, in spawn_offline_cross_val_classification_child_runs:
	for i in range(self.innereye_config.number_of_cross_validation_splits):
self.innereye_config is None

This is the commit message #12:

Parking offline, since it will only work for 1 GPU anyway

This is the commit message #13:

unused param better as _

This is the commit message #14:

unit test for lightningcontainer get_hyperdrive_config

This is the commit message #15:

MyPy fix

This is the commit message #16:

Removing get_total_number_of_cros_validation_runs

Remnant of an old feature

This is the commit message #17:

flake8 fixes

This is the commit message #18:

Expanding abstract method documentation

As per #483 (comment)

This is the commit message #19:

Reverting, mypy errors

How can changing two abstract method's doc-strings cause mypy errors?

This is the commit message #20:

Removing unneeded Optional

This is the commit message #21:

We do need that property

But fixing typing via an exception feels all wrong

This is the commit message #22:

Extending abstract method documentation

This is the commit message #23:

Unit test for cross validation lightning changes to runner

This is the commit message #24:

Adding cross validation to HelloContainer

This is the commit message #25:

Correcting HalloContainer xval splits + comments

This is the commit message #26:

Unit test for xval work in HalloContainer

But test not passing MyPy yet, grrrrrrr

This is the commit message #27:

mypy fixes on unit test

This is the commit message #28:

updating changelog

This is the commit message #29:

testing val sets add up correctly

This is the commit message #30:

finishing documentation

This is the commit message #31:

adding comments

This is the commit message #32:

Refactoring HelloDataset

To remove clumsy init method parameters

This is the commit message #33:

homegrown -> sklearn.model_selection.KFold

This is the commit message #34:

Dropping notimplemented override

This is the commit message #35:

Restoring override in correct place

This is the commit message #36:

Refactoring get_parameter_search_hyperdrive_config

As per Anton's comment

Moving more xval methods into LightningContainer

import fix

resoring get_hyperdrive_config for non lightning models

restoring required sampler method too

reverting cosmetic change

restoring unit test
@dumbledad dumbledad force-pushed the timregan/3814-cross-validation-lightning branch from 491fe76 to 6686a1b Compare June 22, 2021 16:05
ant0nsc
ant0nsc previously approved these changes Jun 23, 2021
View reviewed changes
ant0nsc
ant0nsc previously approved these changes Jun 23, 2021
View reviewed changes
@dumbledad dumbledad requested a review from JonathanTripp June 24, 2021 06:51
@dumbledad dumbledad merged commit c2f0d66 into main Jun 24, 2021
@dumbledad dumbledad deleted the timregan/3814-cross-validation-lightning branch June 24, 2021 19:53
@ant0nsc ant0nsc linked an issue Jun 29, 2021 that may be closed by this pull request
Enable cross validation for LightningContainer models #460
Closed
@dumbledad dumbledad mentioned this pull request Jul 1, 2021
Closed
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@ant0nsc ant0nsc ant0nsc approved these changes

@Shruthi42 Shruthi42 Shruthi42 approved these changes

@javier-alvarez javier-alvarez Awaiting requested review from javier-alvarez

@JonathanTripp JonathanTripp Awaiting requested review from JonathanTripp

Assignees

@dumbledad dumbledad

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable cross validation for LightningContainer models
3 participants
@dumbledad @ant0nsc @Shruthi42