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

🏗️ Not hitting the REST API #127

Open
falexwolf opened this issue Dec 3, 2024 · 2 comments
Open

🏗️ Not hitting the REST API #127

falexwolf opened this issue Dec 3, 2024 · 2 comments

Comments

@falexwolf
Copy link
Member

falexwolf commented Dec 3, 2024

In the latest PR, @lazappi said:

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.

I said:

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.

And meanwhile there is a 3rd and a 4th reason.

  • custom SSL certificate handling (works in lamindb already, would need to be added for the REST endpoint)
  • auto-refreshing the JWT (something that Robrecht and another user encountered early this week)

I fear there might be more issues that will uncover with more usage in more sophisticated scenarios, which will have been resolved in lamindb, and will need to be re-resolved in laminr.

So, yes, I'm now strongly leaning towards refactoring to no longer use the REST API but rather using reticulate consistently.

@falexwolf falexwolf changed the title Not hitting the REST API 🏗️ Not hitting the REST API Dec 3, 2024
@lazappi
Copy link
Collaborator

lazappi commented Dec 4, 2024

I agree. I think we will keep running into things we can't do with the API so we have to wrap Python or come up with some other work around.

  • Relying on Python lamindb means we can avoid re-implementing stuff in R as much as possible so there will be one current implementation (for most things)
  • It might be possible to automatically wrap all the Python functionality. I have some ideas how to do this but I'm not sure it will work and there are probably things I haven't considered. If it works, we could focus on improving docs, UX, fixing bugs rather than having to add each new function.
  • If we know we need Python we can make the setup process smoother to make sure users have a working environment and possibly avoid users needing to go outside R to do anything.

This would be a significant refactor of the package though that would take some time. Given it's already December it might be a project for next year.

@rcannood Probably has other thoughts and/or things I forgot.

@falexwolf
Copy link
Member Author

Yes, let's hear @rcannood -- if we all agree on this, I think we should more or less pause development until there is a week in which you have enough time to do the refactor.

If we know we need Python we can make the setup process smoother to make sure users have a working environment

I think this can already be assumed now. The read-only case is a total edge case that likely almost nobody needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants