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

ENH: Upgrading package versions for security patches #757

Merged
merged 19 commits into from
Sep 14, 2022

Conversation

peterhessey
Copy link
Contributor

@peterhessey peterhessey commented Jul 4, 2022

Upgrading various packages to patch identified critical CVEs.

This involves updating pytorch-lightning to 1.6.4. This introduces a number of breaking changes for various components and tests which have been patched in this PR.

However, this includes two potentially problematic changes:

  • model_training.py now no longer changes the current working directory before calling trainer.fit(). This was as it was causing the CIFAR SSL container tests to fail as CIFAR is downloaded to /None/ in the head of the dir. I'm personally of the opinion that we shouldn't be changing the working directory anyway as it's a perfect recipe for annoying problems like this one (took me a long time to figure out what was going on). I think it should be the responsibility of the user running non-InnerEye models to ensure that their files are being saved to the appropriate folder, not IE-DL's job to try and guess where they're going to go.
  • Updated expected_metrics in test_innereye_ssl_container_cifar10_resnet_simclr(). These regression tests were failing after updating the package versions so I updated the expected metrics to the new ones. The validation loss values had improved, the training ones worsened, neither significantly. This could affect other models but I'm not sure which models need to be checked!

Also includes an upgrade to hi-ml v0.2.5, closing #801

@peterhessey peterhessey changed the title Upgrading package versions for security patches ENH: Upgrading package versions for security patches Jul 4, 2022
@peterhessey peterhessey force-pushed the phessey/critical-security-package-upgrades branch 6 times, most recently from 1d02425 to e14ea6d Compare July 11, 2022 08:46
@peterhessey peterhessey force-pushed the phessey/critical-security-package-upgrades branch from e14ea6d to 2e60a09 Compare July 12, 2022 08:29
@peterhessey peterhessey force-pushed the phessey/critical-security-package-upgrades branch 2 times, most recently from 949bc0c to 54ce6b1 Compare August 23, 2022 13:33
@peterhessey peterhessey force-pushed the phessey/critical-security-package-upgrades branch from 54ce6b1 to 410a33e Compare August 24, 2022 08:16
@peterhessey peterhessey marked this pull request as ready for review August 25, 2022 13:58
@fepegar fepegar removed their request for review August 30, 2022 10:47
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.

We can probably simplify the PR by removing conda merge. Also, I have a couple of questions about your lightning changes.

InnerEye/Common/common_util.py Show resolved Hide resolved
InnerEye/ML/SSL/lightning_modules/ssl_online_evaluator.py Outdated Show resolved Hide resolved
InnerEye/ML/configs/other/fastmri_varnet.py Show resolved Hide resolved
InnerEye/ML/lightning_base.py Show resolved Hide resolved
InnerEye/ML/model_training.py Show resolved Hide resolved
Tests/ML/utils/test_model_util.py Show resolved Hide resolved
Copy link

@kenza-bouzid kenza-bouzid left a comment

Choose a reason for hiding this comment

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

LGTM, left minor suggestions/clarification questions

primary_deps.yml Outdated Show resolved Hide resolved
InnerEye/ML/configs/other/fastmri_varnet.py Show resolved Hide resolved
Tests/SSL/byol/test_byol_moving_average.py Show resolved Hide resolved
Tests/ML/utils/test_model_util.py Show resolved Hide resolved
Tests/ML/utils/test_model_util.py Show resolved Hide resolved
@peterhessey peterhessey enabled auto-merge (squash) September 14, 2022 16:13
@peterhessey peterhessey merged commit 5b21840 into main Sep 14, 2022
@peterhessey peterhessey deleted the phessey/critical-security-package-upgrades branch September 14, 2022 16:33
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