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

✨ Track artifacts as inputs #124

Merged
merged 6 commits into from
Dec 2, 2024
Merged

✨ Track artifacts as inputs #124

merged 6 commits into from
Dec 2, 2024

Conversation

lazappi
Copy link
Collaborator

@lazappi lazappi commented Dec 2, 2024

Related to: Fixes #98

Description

  • Add Python lamindb to all instances (if available)
    • Still warn for the default instance if not
  • Use Python in Artifact$cache() (if available)
    • Fall back to the API if not with a warning

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

@lazappi
Copy link
Collaborator Author

lazappi commented Dec 2, 2024

Example of doing this in an R script https://lamin.ai/laminlabs/lamindata/transform/Q5qWerxOc1R30000/lOBfMDTlIfZMeT4zvsza

@falexwolf I noticed the website says the code is Python and some of the formatting in the code is messed up (mostly <- gets converted to &lt;-)

@lazappi lazappi requested a review from rcannood December 2, 2024 09:24
@falexwolf
Copy link
Member

@falexwolf I noticed the website says the code is Python and some of the formatting in the code is messed up (mostly <- gets converted to <-)

This is solved in staging. Will go to prod later today.

@falexwolf
Copy link
Member

Amazing that this seems to work already! 🤩

Looking through the code here now.

#'
#' @return The path to the cached artifact
cache = function() {

py_lamin <- private$.instance$get_py_lamin()
if (!is.null(py_lamin)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there every a case in which the Python package is not available? We already needed it for lamin connect etc. -- so I think it's always going to be available?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessarily -- if the user provides a correct current_user.env they could still connect to a lamindb instance without the python client. they just won't be able to track, create or delete records at this stage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess you need to CLI installed somewhere to do that but it doesn't necessarily mean it's an environment that {reticulate} can find.

Copy link
Member

Choose a reason for hiding this comment

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

Ok!

@falexwolf
Copy link
Member

Great, @lazappi!

Does artifact$load() call artifact$cache() under-the-hood? If so, this is good to merge from my side.

Copy link
Collaborator

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

LGTM!

@lazappi
Copy link
Collaborator Author

lazappi commented Dec 2, 2024

Does artifact$load() call artifact$cache() under-the-hood? If so, this is good to merge from my side.

Yep. $cache() is used to get the file path and then we read it on the R side. That's easier than trying to read it in Python and convert to R.

@lazappi lazappi merged commit 362d285 into main Dec 2, 2024
7 checks passed
@lazappi lazappi deleted the issue-98/track-inputs branch December 2, 2024 10:13
@falexwolf falexwolf changed the title ✨ Tracking artifacts loaded from other instances ✨ Tracking artifacts as inputs Dec 2, 2024
@falexwolf falexwolf changed the title ✨ Tracking artifacts as inputs ✨ Track artifacts as inputs Dec 2, 2024
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.

Fix tracking run inputs
3 participants