Skip to content
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

MedGen build error #24

Merged
merged 1 commit into from
Apr 28, 2024
Merged

MedGen build error #24

merged 1 commit into from
Apr 28, 2024

Conversation

joeflack4
Copy link
Contributor

@joeflack4 joeflack4 commented Apr 28, 2024

addresses: #23

Changes

  • Updated ODK container reference in GH action to latest

@joeflack4 joeflack4 changed the title - Bug fix: ImportError: on importing MappingSetDataFrame, even though… MedGen build error Apr 28, 2024
@joeflack4 joeflack4 self-assigned this Apr 28, 2024
@joeflack4 joeflack4 added the bug Something isn't working label Apr 28, 2024
src/utils.py Outdated
@@ -5,7 +5,7 @@
import curies
import pandas as pd
import yaml
from sssom import MappingSetDataFrame
from sssom.util import MappingSetDataFrame
Copy link
Contributor Author

@joeflack4 joeflack4 Apr 28, 2024

Choose a reason for hiding this comment

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

Failed attempt 1: update import path

As I wrote in the issue, ense, it is strange that __init__.py imports MappingSetDataFrame, so importing from sssom rather than sssom.util should also work. In any case, I tried this and it didn't fix the problem.

@joeflack4 joeflack4 force-pushed the bugfix-build branch 2 times, most recently from 368724e to 298814a Compare April 28, 2024 04:26
… version of sssom-py in action is same as local env, and the variable exists in __init__.py.

  - attempt 2: upgrade sssom-py
  - attempt 3: update odk container to latest
@@ -11,7 +11,7 @@ on:
jobs:
build_and_release:
runs-on: ubuntu-latest
container: obolibrary/odkfull:v1.4.1
container: obolibrary/odkfull:latest
Copy link
Contributor Author

@joeflack4 joeflack4 Apr 28, 2024

Choose a reason for hiding this comment

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

The fix

This was the problem. This older version of the production ODK container was not up-to-date enough.

Advantages to latest > fixed version

  • Have access to the latest features and fixes.
  • Don't have to worry about checking that the version is up-to-date / correct whenever work is done on the repo.

Disadvantages

  • Can introduce breaking changes (it's because of this that I chose a fixed version before)

Alternatives? (probably not)

It could be nice to use a wildcard * on the minor or patch version, but not the major version. However, upon some googling, generative AI suggested I can do this, but search results and my terminal (invalid reference format) show that I cannot. That makes sense. I think container tags are only string labels; doesn't support a proper versioning system like semver.

Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer fixed versions to minimise churn from breaking changes. These happen more frequently as python code ages..

But I dont mind strongly in this case.

Copy link
Member

Choose a reason for hiding this comment

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

(Note that we may not be monitoring dozens of repos with actions which we rely on to supply as with up to date data. If one of them stops working, maybe we would never know).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After I merged this, I thought about it later and I think I agree with you. I made a new PR and fixed it to 1.5:

If one of them stops working, maybe we would never know).

@matentzn Just to clarify. Are you saying that this is a risk of setting the version to latest? And, additionally, are you just not sure if (the appropriate) people are receiving (and actually taking seriously / reviewing) notifications for GH action failures?
For me, I see them as an email and I always review, but IDK about others.

Copy link
Member

Choose a reason for hiding this comment

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

And, additionally, are you just not sure if (the appropriate) people are receiving (and actually taking seriously / reviewing) notifications for GH action failures?

I can tell you for myself that I receive hundreds of notifications daily and I will miss many; and I know that compared to many people I know that are even more busy that they will be guaranteed to miss theirs.

Copy link
Member

Choose a reason for hiding this comment

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

. Are you saying that this is a risk of setting the version to latest?

Using a versioned container should minise the risk of failure, as no upstream tool changes can impact the execution.

@joeflack4 joeflack4 requested review from matentzn and twhetzel April 28, 2024 04:36
@joeflack4 joeflack4 linked an issue Apr 28, 2024 that may be closed by this pull request
@joeflack4 joeflack4 merged commit 6821359 into main Apr 28, 2024
1 check passed
@joeflack4 joeflack4 deleted the bugfix-build branch April 28, 2024 04:37
joeflack4 added a commit that referenced this pull request Apr 28, 2024
@joeflack4 joeflack4 mentioned this pull request Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MedGen build error
2 participants