-
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
✨ Track artifacts as inputs #124
Conversation
Only warn for the default instance
Fall back to API if not
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 |
This is solved in staging. Will go to prod later today. |
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)) { |
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.
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?
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.
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.
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 guess you need to CLI installed somewhere to do that but it doesn't necessarily mean it's an environment that {reticulate} can find.
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.
Ok!
Great, @lazappi! Does |
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.
LGTM!
Yep. |
Related to: Fixes #98
Description
Artifact$cache()
(if available)Checklist
Before review
Before merge
Update architecture vignetteREADME
CHANGELOG