Added S3 support for datasets, refactored asset loaders

mozilla-ai / lm-buddy

Your buddy in the (L)LM space.

Apache License 2.0

63 stars 3 forks source link

Added S3 support for datasets, refactored asset loaders #99

Closed aittalam closed 6 months ago

aittalam commented 6 months ago

What's changing

Added S3 support for datasets
Refactored the asset loaders to (1) support S3 datasets and (2) be asset-specific. We now have one class for datasets, one for models/configs, one for tokenizers.

How to test it

Used pytest for backwards compatibility.
Manually tested S3 dataset loading in the prometheus job (we might want to rely on localstack for testing)

aittalam commented 6 months ago

I have one comment to consider regarding asset loader classes vs helper functions. It's your call, so if you don't want to make the changes I can approve after your response.

Thanks Sean! As S3 support for models & co (basically anything which is not a dataset) has a dependency on an s3 client, I think it makes sense to keep this (even if I agree with you that the function approach looks cleaner!).

sfriedowitz commented 6 months ago

@aittalam whoops! Add a version bump before merging please.

aittalam commented 6 months ago

@aittalam whoops! Add a version bump before merging please.

Great catch! 🙏 :-)