-
Notifications
You must be signed in to change notification settings - Fork 3
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
Upgrading peotry lock to include jinja update. #308
Conversation
updates: - [github.com/pre-commit/mirrors-mypy: v1.13.0 → v1.14.0](pre-commit/mirrors-mypy@v1.13.0...v1.14.0)
…a vulnerability in that library
[pre-commit.ci] pre-commit autoupdate
…titute/FL4Health into dbe/fixing_jinja_vulnerability
@@ -29,6 +29,7 @@ grpcio = "^1.60.0,!=1.64.2,!=1.65.1,!=1.65.2,!=1.65.4" | |||
monai = "^1.3.0" | |||
nnunetv2 = "^2.3.1" | |||
wandb = "^0.18.0" | |||
acvl_utils = "0.2" # Pin as it was causing an issue with nnunet (ModuleNotFoundError: No module named 'blosc2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was causing an issue with nnunet. See here: https://discourse.slicer.org/t/totalsegmentator-failed-to-compute-results-returned-non-zero-exit-status-1/38142/7 for others who were also having this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed something similar on my end as well. I've been resolving it just by manually pip installing blosc2. Is it better to pin acvl_utils or should blosc2 be added as a dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm...that's a good question. Has everything been working alright for you with forcing a blosc2 install? If so, that might be a better option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, have you pinned the blosc2
library to anything specific, or is it just a free dependency for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to lock blosc2
if that works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't pin it to a version, just free installed it. I currently have version 3.0.0b4 and everything ran fine for my application. Can't confirm whether or not it will cause issues elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. So I tried this out. Unfortunately, blosc2
3.0.0 requires python 3.11 (we're 3.10 at the moment). So I have to install 2.7.1, which doesn't fix the issue that we're seeing with nnunet and the smoke tests. We'll eventually upgrade to python 3.11, but it may or may not be a big change. So I'm going to revert back to pinning acvl_utils
for now and we'll fix it the right way when we move up to python 3.11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My acvl_utils resolved to 0.2 so I haven't ran into this yet. Seems like the right call is just to pin acvl_utils for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes my bad, that was a different environment. I actually had blosc2 version 2.7. Happy with either solution though. Maybe we should create an issue within nnunet asking them to update their dependencies
…talling blosc2 instead of pinning acvl_utils
…blosc2 3.0.0 with it.
clipping_bound = float(packed_parameters[split_size:][0]) | ||
return model_parameters, clipping_bound | ||
clipping_bound = packed_parameters[split_size:][0] | ||
return model_parameters, clipping_bound.item() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needed to be updated as it was throwing a deprecation error that was killing the smoke tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
PR Type
Fix
Short Description
Updating the poetry lock file. This includes moving to Jinja 3.1.5, which is to fix two vulnerabilities in that library
Tests Added
N/A