-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update feedstock and recipe #14
base: main
Are you sure you want to change the base?
Conversation
…nda-forge-pinning 2024.10.24.16.28.23
recipe/meta.yaml
Outdated
@@ -27,9 +27,9 @@ requirements: | |||
- pip | |||
run: | |||
- python >=3.8,<3.12 | |||
- tiledbsoma-py | |||
- tiledbsoma-py >=1.11.4,<1.12.0 |
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 think for cloud we want maximum flexibility to decouple from census releases
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 agree that all things being equal we want maximum flexibility. But why does the strict requirement exist upstream if it isn't actually a requirement? Do we want to deploy versions of cellxgene-census to cloud that haven't been tested against the version of tiledbsoma-py currently installed in cloud? Updates to tiledbsoma-py can and do break cellxgene-census (eg this example from last month chanzuckerberg/cellxgene-census#1288)
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.
After sleeping on it, I think I understand the problem better. If we bump the minor version of tiledbsoma-py and want to deploy this to cloud, we wouldn't be able to without also updating/releasing a new version of cellxgene-census.
I really like using pip check
to confirm the dependencies are correct. My current idea for a workaround is to patch cellxgene-census to remove its strict requirement on tiledbsoma-py.
Though this does raise larger questions:
- Why is cellxgene-census so linked to tiledbsoma-py? Is it an independent package, or should somacore/tiledbsoma-py/cellxgene-census be viewed more as a single entity? Ideally minor version bumps of tiledbsoma-py should not break downstream dependencies
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 current idea for a workaround is to patch cellxgene-census to remove its strict requirement on tiledbsoma-py.
Local code I ran to generate the patch, in case we need to regenerate it for future releases:
wget https://files.pythonhosted.org/packages/eb/cf/4b0ec066b39cc3b020f1fb7c754c1135a0c53467b9703fe71131b9320a3a/cellxgene_census-1.15.0.tar.gz
tar xzf cellxgene_census-1.15.0.tar.gz
cd cellxgene_census-1.15.0/
sed -i "s/tiledbsoma~=/tiledbsoma>=/" pyproject.toml
git diff > remove-tiledbsoma-upper-bound.patch
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.
Why is cellxgene-census so linked to tiledbsoma-py? Is it an independent package, or should somacore/tiledbsoma-py/cellxgene-census be viewed more as a single entity? Ideally minor version bumps of tiledbsoma-py should not break downstream dependencies
It depends crucially on tiledbsoma. The main issue is storage-format changes in core -- which are infrequent these days.
- For their
pyproject.toml
they should be able to pin as they wish (their package) - Empirically we find that there is significant lag between our bumping a tiledbsoma version and them bumping their pin on tiledbsoma
- They can bump when they're ready in their
pyproject.toml
for their PyPI package - Sometimes (usually) we need to get a new core + other packages + tiledbsoma version onto cloud before they've bumped in their
pyproject.toml
-- it's not acceptable to delay our cloud releases until CZI has done their pin-bump process
- They can bump when they're ready in their
- This feedstock is only "for cloud and for us"
- We loosen the pin here in this feedstock
- We take it upon ourselves to do notebook QA to 'vet' that new tiledbsoma works with whichever version of cellxgene
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.
@johnkerl thanks for the added explanation. I think we're on the same page now.
Do you approve of my workaround for this feedstock? I kept the lower bound, but I removed the upper bound from the conda recipe as well as from the source pyproject.toml
via a recipe patch. I don't want to self-merge without your approval.
While troubleshooting #12, I noticed that the feedstock and recipe would benefit from a refresh.
Feedstock:
Recipe:
--no-deps --no-build-isolation
to ensure all dependencies are installed from condapip check
, which identified problems with the numpy and tiledbsoma-py boundstiledbsoma~=1.11.4
chanzuckerberg/cellxgene-census#1187 is equivalent to bounds of >=1.11.4,<1.12.0I wasn't originally planning to bump the build number, but since I did edit the requirements, I did bump it to upload a new binary.
Here's the output from
pip check
that identified the requirements that needed updating: