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 use_ajax flag to MLFlow instances #44

Merged
merged 4 commits into from
May 17, 2024

Conversation

pebeto
Copy link
Member

@pebeto pebeto commented Apr 9, 2024

Alowing compatibility with both REST and AJAX endpoints.

@pebeto pebeto added the enhancement New feature or request label Apr 9, 2024
@pebeto pebeto self-assigned this Apr 9, 2024
@pebeto pebeto linked an issue Apr 9, 2024 that may be closed by this pull request
@stemann
Copy link
Contributor

stemann commented Apr 9, 2024

Are there any other MLFlow implementations/deployments using /ajax-api other than DagsHub? (I have only seen mention of /ajax-api in #35 (comment))

Wouldn't a more natural API be to provide the API base url, e.g. http://localhost:5000/api or https://dagshub.com/nirbarazida/CheXNet.mlflow/ajax-api (?), instead of providing the MLFlow UI base url (http://localhost:5000) and having a kwarg with a not-very-descriptive name (use_ajax) ?

@pebeto
Copy link
Member Author

pebeto commented Apr 9, 2024

I don't know if there's other than DagsHub.

I consider the API base URL approach as a good one. With that, we don't need to be aware of if it's an ajax-api or api.

Need opinions about this decision.
@ablaom @deyandyankov

@deyandyankov
Copy link
Collaborator

I agree with the base URL approach, but it will break compatibility for existing users. I.e. they will need to update their URLs if we make that change and they upgrade to a newer release of the package.

@ablaom
Copy link
Member

ablaom commented Apr 9, 2024

No objections but this definitely needs to be tagged breaking. So if we have any other breaking changes in mind...

@svilupp
Copy link
Contributor

svilupp commented Apr 12, 2024

I'm very excited about this PR! I was about to open a new PR myself, as the latest version is broken for us (past ones work).

The following commit broke the ability to work with Databricks: e27ac10

We use Databricks-managed MLFlow and we need the "HOST/api/2.0" format rather than the current "/ajax-api" one.

The proposed approach to provide the base URL including the relevant API path would work great for us!

@stemann
Copy link
Contributor

stemann commented Apr 15, 2024

No objections but this definitely needs to be tagged breaking. So if we have any other breaking changes in mind...

A very much related breaking change would be to change the type of apiversion::Union{Integer, AbstractFloat} to, e.g. api_version::VersionNumber.

@svilupp
Copy link
Contributor

svilupp commented Apr 23, 2024

@pebeto Would it be, please, possible to merge this PR and tag a new version?

Given that the commit linked above was breaking (anything above v0.4.4), anyone who updates (even if having compat to "0.4") might get the same error as we did.

It would be good to have a clean new tag.

@svilupp
Copy link
Contributor

svilupp commented May 17, 2024

@pebeto Would you please consider merging the PR to have a working minor version release?

The bug introduced in 0.4.5 (the ajax in path) still persists.

@pebeto pebeto force-pushed the 43-support-gitlabs-mlflow-backend branch from 275316a to 66a9359 Compare May 17, 2024 14:29
@pebeto pebeto merged commit 399f526 into main May 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support GitLabs MLFlow backend
5 participants