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

✨Add support for local source files #156

Merged
merged 11 commits into from
Nov 19, 2024

Conversation

mossjacob
Copy link
Collaborator

This is a very small PR and I'm definitely open to better ways of doing this!

Some use-cases, e.g. debugging, may require local files to be loaded as a source.

As an example, I added to sources_local.yaml the following lines:

Gene:
  ensembl:
    arabidopsis thaliana:
      release-57:
        url: file:///path/to/local/df_arabidopsis thaliana__ensembl__release-57__Gene.parquet

With the change in this PR I can load this local parquet file. There's almost certainly a better way of doing this so please do enlighten me!
Thanks

@mossjacob mossjacob changed the title initial support for local files Add support for local files Nov 14, 2024
Copy link
Member

@sunnyosun sunnyosun left a comment

Choose a reason for hiding this comment

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

This looks good! Could you add a small test for it?

@sunnyosun
Copy link
Member

sunnyosun commented Nov 14, 2024

Just letting you know that we are working on a better process than modifying the sources_local.yaml, all the files that you curated will be all registered in your lamindb instance and tracked there. We'll update you with the new process once we are there!

@mossjacob
Copy link
Collaborator Author

Will add a test probably on Monday! Thanks for letting me know, sounds great.

@Zethson
Copy link
Member

Zethson commented Nov 15, 2024

I'm sorry for abusing this PR to ensure that the CI now runs fine on forked PRs but it does now. So with your test on Monday, we can merge this although this design is going to be succeeded in the future.

Thank you very much!

@mossjacob mossjacob marked this pull request as ready for review November 19, 2024 16:16
@mossjacob
Copy link
Collaborator Author

Have now added the test! thanks

Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
@Zethson
Copy link
Member

Zethson commented Nov 19, 2024

Thank you so much! I made a few edits myself to save you a bit of time and get this in more quickly. I hope you don't mind. If you'd rather have me review smaller PRs instead of making changing to them, please let me know!

  1. I ran pre-commit. We are using it on all of our repositories and I'd encourage you to use it for your work and all PRs that you make to our repositories. See https://pre-commit.com/
  2. We should not hardcode paths like /tmp because they might not be writable on Windows devices or clusters. We are now using tempfile to create temporary test files.

@Zethson Zethson changed the title Add support for local files ✨Add support for local source files Nov 19, 2024
@Zethson Zethson merged commit 790fd1e into laminlabs:main Nov 19, 2024
3 checks passed
@mossjacob
Copy link
Collaborator Author

No that's perfect, thanks so much for making those improvements. Completely understood re pre-commit and tempfile!

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