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

Remove poetry lockfile to better project testing with scheduled trigger #170

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

d33bs
Copy link
Member

@d33bs d33bs commented Mar 18, 2024

Description

This PR is an addendum to #167 to assist with scheduled projected testing. The intent here would be to remove the lockfile in case there are dependencies which users may see upstream that we don't yet see in CytoTable testing due to the lockfile. For example, in the case of #163 testing would not have failed for scheduled tests due to the lockfile avoiding the newer versions of duckdb.

Thanks for any thoughts or feedback you may have on this!

What is the nature of your change?

  • Bug fix (fixes an issue).
  • Enhancement (adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • This change requires a documentation update.

Checklist

Please ensure that all boxes are checked before indicating that a pull request is ready for review.

  • I have read the CONTRIBUTING.md guidelines.
  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • New and existing unit tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have deleted all non-relevant text in this pull request template.

@kenibrewer
Copy link
Member

Hmmm.... I'm not sure removing the the lock file is the appropriate fix here. I'll dig into this a bit later with my rationale.

@d33bs
Copy link
Member Author

d33bs commented Mar 18, 2024

Thanks @gwaybio and @kenibrewer ! More background, in case it helps, what I've been seeing lately is that we incur "surprise" dependency related challenges due to the poetry.lock file and the differences in time alignment between PR's vs external release timelines. This is intended to be a proactive and observable way to detect these possibly before users experience them. Ideally, if we can catch them and fix them quickly enough, users would never see these challenges in their own work.

@kenibrewer
Copy link
Member

Here's what I'm confused about. According to the description of the poetry lock file, the lock file is intended to prevent exactly this type of scenario that occurred with #163. It provides a specific list of allowed versions for each dependency.

It might be helpful understand a bit more about how exactly the incompatible version of duckdb was installed. Did the poetry.lock get updated to allow duckdb 0.10.0 even though we didn't test against it? Is the information from poetry.lock file not kept when building the package for pip and/or conda?

@d33bs
Copy link
Member Author

d33bs commented Mar 19, 2024

Hi @kenibrewer , thanks for your reply on this. The user who reported #163 experienced the issue because of a loose constraint on duckdb from within the pyrproject.toml file which they received through a non-poetry.lock'ed installation. The poetry.lock file did its job within the context of the source code for CytoTable, providing a reproducible testing experience. However, the user still ran into the issue and opted to report the challenge to us for awareness and a fix. The issue occurred because external dependencies are updated outside of existing triggers for CytoTable.

This change leans towards CytoTable developers having a proactive awareness of issues like the one experienced in #163 . It's an experiment towards improved user experience through increased visibility (and an implied developer responsibility as a result).

Copy link
Member

@kenibrewer kenibrewer left a comment

Choose a reason for hiding this comment

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

@d33bs Thanks for the additional clarification. I didn't realize that only the version constraints from the pyproject.toml are used in some downstream installation methods. LGTM

@kenibrewer kenibrewer assigned kenibrewer and unassigned kenibrewer Mar 19, 2024
@d33bs
Copy link
Member Author

d33bs commented Mar 19, 2024

Thanks @kenibrewer !

@d33bs d33bs merged commit 0e4cf6b into cytomining:main Mar 19, 2024
7 checks passed
@d33bs d33bs deleted the scheduled-testing-rm-poetry-lock branch March 19, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants