mlp icon indicating copy to clipboard operation
mlp copied to clipboard

feat(mlflow): Add s3 artifact type

Open deadlycoconuts opened this issue 1 year ago • 2 comments

Context

Given the need to support other Mlflow registries other than Google Cloud Storage, this PR introduces support for S3-based registries to the artifact package, allowing other CaraML components such as Merlin and Turing to work with Mlflow that use S3-based artifact registries.

Main Changes

  • api/pkg/artifact/artifact.go - Created a new interface SchemeInterface and added a new implementation of the Service interface (S3ArtifactClient)

Other Modifications

To introduce support for S3, the AWS Golang SDK (version 2) will be introduced as a new dependency of MLP. This change requires updating the version of Go from 1.20 to 1.22, which further requires upgrades to our linter versions (which now report more errors than before), GitHub actions, etc. This PR thus includes those changes which may seem irrelevant to the original scope of this PR as described in its title.

Here's a list of some of those changes:

  • Updating the version of Go to 1.22
  • Updating the version of golangci-lint-action to v6 and the golangci-lint to v1.58.1
  • Deleting of deprecated linters
  • Naming unused arguments as _

Random comment: If anyone finds the naming of client/service a little confusing in this artifact package, we can rename them in this PR also (I'm open for any suggestions 😅).

deadlycoconuts avatar Sep 09 '24 07:09 deadlycoconuts

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 37.87%. Comparing base (9b92012) to head (f6e5f3e).

:exclamation: There is a different number of reports uploaded between BASE (9b92012) and HEAD (f6e5f3e). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (9b92012) HEAD (f6e5f3e)
api-test 2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #116       +/-   ##
===========================================
- Coverage   56.39%   37.87%   -18.53%     
===========================================
  Files          48       68       +20     
  Lines        2477     3976     +1499     
===========================================
+ Hits         1397     1506      +109     
- Misses        884     2263     +1379     
- Partials      196      207       +11     
Flag Coverage Δ
api-test 37.87% <ø> (-18.53%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 09 '24 08:09 codecov[bot]

It seems like the codecov report is getting a lot of unexpected coverage drops and I think it might be due to the way we are using codecov: https://docs.codecov.com/docs/unexpected-coverage-changes. I don't think I'd be able to identify and resolve the root cause in the scope of this PR though; I'll just log this as a card and then potentially remove the codecov CICD job temporarily so that it doesn't make the entire CICD pipeline appear like it failed (when in reality it's due to an unexpected codecov job failure).

deadlycoconuts avatar Sep 25 '24 02:09 deadlycoconuts

Thanks for the review @tiopramayudi ! I'll be merging this PR shortly!

deadlycoconuts avatar Oct 10 '24 04:10 deadlycoconuts