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 an artifact loader for .yaml #2172

Closed
wants to merge 2 commits into from

Conversation

rcannood
Copy link

  • Fix incorrect documentation for load_image
  • Add loaders for .gif and .yaml.

In addition, we could add a data loader for .rds, which when called, produces an error message that you should use laminr for loading .rds files. WDYT?

@rcannood
Copy link
Author

FYI -- laminlabs/laminr#81 added a data loader for .rds.

@falexwolf falexwolf changed the title ✨ Add data loaders ✨ Add artifact loaders Nov 18, 2024
@falexwolf
Copy link
Member

Thanks, Robrecht! Great!

In addition, we could add a data loader for .rds, which when called, produces an error message that you should use laminr for loading .rds files. WDYT?

That's a nice idea, yeah!

@falexwolf
Copy link
Member

We need a test here for the new .yaml artifact loader:

def test_load_to_memory(tsv_file, zip_file, fcs_file):

@Koncopd, do you want to finalize and rename the test function to test_artifact_loaders()?

@falexwolf falexwolf changed the title ✨ Add artifact loaders ✨ Add an artifact loader for .yaml Nov 18, 2024
@Koncopd
Copy link
Member

Koncopd commented Nov 18, 2024

Do we want to add pyyaml to the deps?

@falexwolf
Copy link
Member

No, I'd not add pyyaml to the dependencies because we have too many anyway and nothing in lamindb requires it.

@Koncopd
Copy link
Member

Koncopd commented Nov 19, 2024

Ok, i wanted to push some things here but again we have the problem with CI for PRs not from this repo.

@Koncopd
Copy link
Member

Koncopd commented Nov 19, 2024

Thanks, Robrecht! Great!

In addition, we could add a data loader for .rds, which when called, produces an error message that you should use laminr for loading .rds files. WDYT?

That's a nice idea, yeah!

@rcannood @falexwolf i think a warning is more reasonable for that #2188

@@ -107,9 +108,17 @@ def load_json(path: UPathStr) -> dict:
data = json.load(f)
return data

def load_yaml(path: UPathStr) -> dict:
"""Load `.yaml` to `dict`."""
import yaml
Copy link
Member

Choose a reason for hiding this comment

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

Maybe except ImportError and log a warning that the package is not installed and return a path.

@Koncopd
Copy link
Member

Koncopd commented Nov 27, 2024

load_yaml needs a test.

There is also a comment from me, otherwise good to merge when the comments are resolved.

@Koncopd
Copy link
Member

Koncopd commented Dec 10, 2024

This looks inactive, so moving it here #2270

@Koncopd Koncopd closed this Dec 10, 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.

3 participants