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

🐛 Fix CI setup #125

Merged
merged 25 commits into from
Dec 3, 2024
Merged

🐛 Fix CI setup #125

merged 25 commits into from
Dec 3, 2024

Conversation

rcannood
Copy link
Collaborator

@rcannood rcannood commented Dec 2, 2024

Related to: laminlabs/laminhub-public#29

Description

  • Make sure IPython is installed on the CI (circumvents lamindb fails when IPython is not installed laminhub-public#29)
  • Install wetlab and bionty when rendering the website to make sure warnings are not triggered.
  • Switch to smaller MkRm3eUKPwfnAyZMWD9v artifact when showcasing laminlabs/cellxgene.
  • Make the error message when reticulate::import("lamindb") fails slightly more user friendly by instructing the user to also run reticulate::py_last_error().

Checklist

Before review

  • Update and regenerate man pages
  • Add/update tests
  • Add/update examples in vignettes
  • Pass CI checks

Before merge

  • Update architecture vignette
  • Update development vignette
  • Update features in README
  • Update CHANGELOG

@falexwolf
Copy link
Member

If this resolves things, it'd be great to merge ASAP.

@falexwolf
Copy link
Member

Ah, but looking at it it appears like the CI would be inconsistent with what the user reads. So, this seems dangerous.

@falexwolf
Copy link
Member

falexwolf commented Dec 2, 2024

The package should work just with lamindb installed, without need of bionty or wetlab.

Something very recent must have introduced these warnings and it's a shame that we didn't catch them. If lamindb isn't available, I'd say, we should hard error going forward.

In retrospect, I should have kept digging about the below this morning:

image

While having lamindb not be discovered by reticulate is possible, I'm now pretty convinced it opens up a dangerous edge-case mode for using the package that then needs to be tested in addition to "conventional usage", which heavily relies on reticulate and lamindb.

Also looping in @lazappi into the discussion.

@rcannood
Copy link
Collaborator Author

rcannood commented Dec 2, 2024

Ah, but looking at it it appears like the CI would be inconsistent with what the user reads. So, this seems dangerous.

I think the pkgdown CI should install these packages to trigger the least amount of warnings. The unit test CI on the other hand should be able to work without bionty and wetlab.

Something very recent must have introduced these warnings and it's a shame that we didn't catch them. If lamindb isn't available, I'd say, we should hard error going forward.

The warning related to bionty and wetlab not being installed are coming from python, though.

The other one about reticulate not being able to import lamindb via reticulate is one I wasn't able to replicate locally so far. I was trying to reproduce it via this PR.

@falexwolf
Copy link
Member

I think the pkgdown CI should install these packages to trigger the least amount of warnings.

I think I disagree. If the docs don't show warnings and users see them they'll be worried and confused. I'd rather say: "it's Ok to ignore these warnings; they're only relevant if you use the Python package".

Background: I was planning on removing the warnings about bionty & wetlab on the lamin CLI level but then didn't find a way how to tell that the user won't be affected if they're using laminr -- evidently the CLI is used by Python & R users in the same way. Now that we started this discussion about reticulate etc. I'm also not sure whether we will eventually want this warning. So, I'd keep it around until we're sure or find a way to disambiguate.

The warning related to bionty and wetlab not being installed are coming from python, though.

These warnings have always been there and make sense if you continue to use lamindb as a Python package after connecting via the CLI.

The other one about reticulate not being able to import lamindb via reticulate is one I wasn't able to replicate locally so far. I was trying to reproduce it via this PR.

This is what I'm talking about! This is a new thing and looks worrisome to me.

@lazappi
Copy link
Collaborator

lazappi commented Dec 3, 2024

{reticulate} being able to find the correct Python installation is one of the trickier things about using it. If you have everything installed in whatever your base system Python is then it should be fine but if lamindb is installed in a conda/virtualenv environment then you need to make sure to point {reticulate} to that before loading any Python packages.

I have lamindb only in an environment so it won't be found unless I use reticulate::use_virtualenv(...) or similar first (like in some of the scripts I uploaded). There are things you can do to make this easier for users like described here but we haven't looked into it yet.

This is one of the reasons I think we should discuss switching fully to {reticulate} and dropping the API access, so we can make Python availability a hard dependency.

@falexwolf
Copy link
Member

{reticulate} being able to find the correct Python installation is one of the trickier things about using it. If you have everything installed in whatever your base system Python is then it should be fine but if lamindb is installed in a conda/virtualenv environment then you need to make sure to point {reticulate} to that before loading any Python packages.

I also learned this playing with laminr on Sunday.

This is one of the reasons I think we should discuss switching fully to {reticulate} and dropping the API access, so we can make Python availability a hard dependency.

Understood. I'm also pretty convinced that this is how it should be done independent of that reason: there is too much logic in lamindb to re-build it here in any finite amount of time, or move behind the REST. I believe that this is why MLFlow and probably several other packages rely on reticulate to build their R clients.

@rcannood rcannood changed the title 🐛 Fix lamindb installation instructions 🐛 Fix CI setup Dec 3, 2024
@rcannood
Copy link
Collaborator Author

rcannood commented Dec 3, 2024

Turns out the reticulate/Python setup was a red herring.

Apparently calling reticulate::import("lamindb") in R or even import lamindb in Python fails if IPython is not installed.

This PR adds a temporary workaround to make sure IPython is installed and also created laminlabs/laminhub-public#29.

@rcannood rcannood requested a review from lazappi December 3, 2024 12:20
@falexwolf
Copy link
Member

Customers are looking at it right now, so I'll merge it.

@falexwolf falexwolf merged commit f5bd317 into main Dec 3, 2024
7 checks passed
@falexwolf falexwolf deleted the fix-lamindb-install branch December 3, 2024 12:59
@falexwolf
Copy link
Member

If changes need to be made, they can go in another PR! ☺️

@falexwolf
Copy link
Member

This PR adds a temporary workaround to make sure IPython is installed

Thanks for quick resolution, Robrecht!

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