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

Change to REST API #5

Merged
merged 11 commits into from
Apr 6, 2024
Merged

Change to REST API #5

merged 11 commits into from
Apr 6, 2024

Conversation

itan1
Copy link
Contributor

@itan1 itan1 commented Apr 3, 2024

I refactored the MLFLogger to use the REST API from MLFlowClient.jl, see issue #3.

I could keep mostly the same interface with only some minor differences.

@itan1 itan1 changed the title Change to rest api Change to REST API Apr 3, 2024
@rejuvyesh
Copy link
Owner

Fantastic. Just one quick thing. The python based mlflow clients usually also handle a lot of cloud authentication stuff (for example logging in azureml is easier if you have azureml-mlflow installed together with mlflow. I'm guessing, we will have to lose on those things if we go the Julia only route until someone implements that part of the API.

@rejuvyesh
Copy link
Owner

Do you wish to take a shot at enabling the server at MLFLOW_URI in the github workflow to allow running the tests. We probably just need to install and run mlflow serve and set the environment variable.

@itan1
Copy link
Contributor Author

itan1 commented Apr 4, 2024

I indeed don't see anything azure specific in MLFlowClient.jl at the moment. Could we make a separate issue for that functionality?

Using the MLFlow basic authentication should be possible via the headers argument like described in this MLFlowClient Issue. The MLFLogger constructor should pass the argument to the MLFlow constructor.

.github/workflows/CI.yml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 78.68%. Comparing base (f59c3b6) to head (80d8609).

Files Patch % Lines
src/MLFlowLogger.jl 80.00% 6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           master       #5       +/-   ##
===========================================
+ Coverage    3.19%   78.68%   +75.49%     
===========================================
  Files           3        1        -2     
  Lines          94       61       -33     
===========================================
+ Hits            3       48       +45     
+ Misses         91       13       -78     

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

@rejuvyesh rejuvyesh merged commit b655a5f into rejuvyesh:master Apr 6, 2024
1 of 3 checks passed
@rejuvyesh rejuvyesh mentioned this pull request Apr 7, 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