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

check that install of "latest dependent packages" of linkml_runtime still results in tests passing (as opposed to running tests only on the poetry.lock dependencies) #288

Closed
wants to merge 2 commits into from

Conversation

sierra-moxon
Copy link
Member

add a check dependencies action that removes the poetry.lock file and reinstalls the linkml-runtime environment (getting the latest compatible packages in the pyproject.toml specification) and then runs the tests as a way of testing the code in a PR on the latest package dependencies

partially resolves #1749.

After discussion during developer days hackathon, we want to limit our downstream build dependencies (e.g. not build out bmt, etc from LinkML packages directly), but makes sense to take this first step towards "latest possible" dependency checking.

… reinstalls the linkml-runtime environment (getting the latest compatible packages in the pyproject.toml specification) and then runs the tests as a round about way of testing the code in a PR on the latest package dependencies
@sierra-moxon sierra-moxon changed the title add a check dependencies action that removes the poetry.lock file and… check that install of "latest dependent packages" of linkml_runtime still results in tests passing (as opposed to running tests only on the poetry.lock dependencies) Dec 1, 2023
Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d45970a) 62.09% compared to head (0579243) 62.09%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #288   +/-   ##
=======================================
  Coverage   62.09%   62.09%           
=======================================
  Files          63       63           
  Lines        8461     8461           
  Branches     2170     2170           
=======================================
  Hits         5254     5254           
  Misses       2599     2599           
  Partials      608      608           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@pkalita-lbl pkalita-lbl left a comment

Choose a reason for hiding this comment

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

The more I think about this the more hesitant I am about it.

Stepping back from linkml/linkml#1749 specifically, there is a question that comes up from time to time about whether we should be committing our poetry.lock file (related: linkml/linkml#1641, linkml/linkml#1567). The Poetry documentation lays it out the concerns pretty concisely. linkml-runtime is clearly a library, and so there's no cut-and-dried answer. We have to consider the trade-offs between reproducibility (by including the lock file) and faster failures due to new dependency versions (by omitting the lock file). Given the limited resources we have to devote to LinkML, we've opted for reproducibility.

I'm concerned that these changes effectively reverse that decision. I'm having hard time imaging a scenario in which this new workflow (running tests after installing without the lock file) passes but the old one (running tests after installing with the lock file) fails. So in that sense why even keep the old one? And in that sense why even commit the lock file? It's totally valid for us to reconsider our stance on the lock file, but we should probably do so in a wider discussion than this PR.

Coming back to linkml/linkml#1749 it isn't even clear to me how this new test workflow could have prevented that situation. While the issue is written to suggest we're lacking testing infrastructure, I don't think that's true. We have the infrastructure; we simply failed to write the proper test case (ahead of time anyway; it has been addressed now) that would have failed and therefore prevent the offending change from being released. Again, I'm happy to reconsider the lock file issue, but we should frame the discussion correctly and I don't think linkml/linkml#1749 is the right framing

@sierra-moxon
Copy link
Member Author

sierra-moxon commented Dec 13, 2023

I agree it's a little bit pushy w/re to upgrade if this action fails. I did use the methodology downstream in biolink-model to find a breaking change in linkml 1.6.4 that wasn't caught by the existing linkml tests, nor was it caught by the poetry.lock version of the biolink-model repo (as that was still at linkml 1.6.3).

The "pushy" part was that in order to get my biolink-model PR to pass this dependency check, I had to update my poetry.lock file in biolink to use linkml 1.6.4 and then change the biolink-model code generation to follow the new pattern introduced in linkml 1.6.4. I'm glad I caught this and I put in a linkml PR to support the old way for awhile so other models downstream don't inadvertently run into this, but it was a bit hard to decide to tie an upgrade of linkml to an unrelated PR so that the PR would pass its tests.

maybe a compromise would be setting continue-on-error: true to the run steps that check downstream dependencies -- that way the submitter gets an email of the failure, but the PR can still go thru

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants