ODK Upgrade #159
ODK Upgrade #159
Conversation
- Upgrade to v1.5.3. Has a bugfix for SSSOM and includes dotenv package.
- Bug fix: Need to add '--user --break-system-packages' due to new nature of ODK security.
There was a problem hiding this comment.
I will check later, but not sure yet why these notebooks are showing up in the diff. They should be on main, not in this PR.
- @joeflack4 figure this out and fix
There was a problem hiding this comment.
yeah, these files should not be here
| @@ -65,7 +65,7 @@ get-pmids: | |||
|
|
|||
| # SETUP / INSTALLATION ------------------------------------------------------------------------------------------------- | |||
| install: | |||
| pip install -r requirements-unlocked.txt | |||
| pip install -r requirements-unlocked.txt --user --break-system-packages | |||
There was a problem hiding this comment.
Build passing now after adding this.
There was a problem hiding this comment.
Why is --user --break-system-packages needed? I thought that was just to add packages that aren't in ODK and ODK should now have everything this repo needs, right?
There was a problem hiding this comment.
Here is the rationale. I'd rather not have to keep adding/removing make install every time I introduce a new package.
There was a problem hiding this comment.
Hmm.... ok. Is it mentioned somewhere, other than a PR comment, that any new non-ODK package needed here does need an ODK issue to request it :)
There was a problem hiding this comment.
I do have it in my 'rules', which I review every now and then, that I personally should open up a GH issue in such an event, but I wouldn't know where to write these things down; maybe some mondo-ingest developer documentation.
Also alternatively, if we wanted to make this make install thing more efficient, we could have it run a script that instead iterates over each package in the requirements.txt, checks first if it is already installed globally, and then only do the pip install PACKAGE --user --break-system-packages only if it is not. Feels a little excessive, but in a way that is the most elegant approach. Right now it's not a major issue.
Never mind, that suggestion was silly. I just remember. Having the make install really doesn't cause any harm. If the package is already there, of course it won't install. E.g. I just checked the action I just ran off of this branch, and it did not need to install anything:
Requirement already satisfied: python-dotenv in /usr/local/lib/python3.12/dist-packages (from -r requirements-unlocked.txt (line 1)) (1.0.1)
| @@ -65,7 +65,7 @@ get-pmids: | |||
|
|
|||
| # SETUP / INSTALLATION ------------------------------------------------------------------------------------------------- | |||
| install: | |||
| pip install -r requirements-unlocked.txt | |||
| pip install -r requirements-unlocked.txt --user --break-system-packages | |||
There was a problem hiding this comment.
Why is --user --break-system-packages needed? I thought that was just to add packages that aren't in ODK and ODK should now have everything this repo needs, right?
There was a problem hiding this comment.
yeah, these files should not be here
resolves #114
resolves #127
Changes