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

Adding S3 Support for Artifacts #34

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

usmcamp0811
Copy link

I have enhanced the logartifact function to accommodate MLFlow instances configured with S3 storage for artifact management. This enhancement includes an added verification step to determine if the specified URI corresponds to an S3 bucket. In cases where the URI is not an S3 bucket, the function retains its original behavior of saving artifacts to the disk. Conversely, when an S3 bucket is identified, the function leverages AWS and Minio services for artifact storage in the bucket. This implementation supports both AWS and Minio, although its compatibility with AWS S3 buckets has not been verified due to the lack of an AWS S3 bucket for testing. However, extensive testing has been conducted with an MLFlow instance utilizing Minio S3 buckets, and the functionality aligns with expected outcomes. It's important to note that when using Minio or similar S3 services, the MLFLOW_S3_ENDPOINT_URL environment variable must be set to ensure proper operation.

@deyandyankov deyandyankov requested a review from pebeto December 29, 2023 10:13
Copy link

codecov bot commented Dec 29, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (5a4237f) 77.64% compared to head (290ac2a) 74.71%.
Report is 3 commits behind head on main.

Files Patch % Lines
src/loggers.jl 42.30% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #34      +/-   ##
==========================================
- Coverage   77.64%   74.71%   -2.94%     
==========================================
  Files           9        9              
  Lines         331      348      +17     
==========================================
+ Hits          257      260       +3     
- Misses         74       88      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@deyandyankov deyandyankov left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

Can you please make a couple of changes:

  • break down logartifact into two underlying functions, one for local disk and one for S3 - and then call them respectively from logartifact based on `!startswith(artifact_uri, "s3://")
  • instead of hardcoding /tmp, use mktemp

Copy link
Member

@pebeto pebeto left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions, @usmcamp0811.

I want to know if having both AWS and AWSS3 is completely necessary. I mean, does not the AWS package do the same as the other one?

Please don't forget to add/edit tests to this change.

@usmcamp0811
Copy link
Author

@pebeto all the docs I looked at for AWS said to use both the packages, but I can try to leave one off.

@deyandyankov yea I can make those changes here soon. I'm pretty busy right now, catching up from the holidays but should be able to get to it this weekend 🤞

@pebeto
Copy link
Member

pebeto commented Mar 7, 2024

You can find Mocking.jl useful to write tests for this specific implementation.

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