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

Fix: Fixed the previous tracking URI setting logic to prevent clashes with original MLflow code. #29096

Merged
merged 5 commits into from
Mar 4, 2024
Merged

Fix: Fixed the previous tracking URI setting logic to prevent clashes with original MLflow code. #29096

merged 5 commits into from
Mar 4, 2024

Conversation

seanswyi
Copy link
Contributor

What does this PR do?

The previous code was calling the mlflow.set_tracking_uri function regardless of whether or not the environment variable MLFLOW_TRACKING_URI is even set. This led to clashes with the original MLflow implementation and therefore the logic was changed to only calling the function when the environment variable is explicitly set.

Fixes # (issue) Doesn't fix a specific issue but addresses a problem that was brought up as a comment in the aforementioned PR: #29032 (comment)

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

Maintainers: @amyeroberts @muellerzr @pacman100
Original author of MLflowCallback: @noise-field
Author of comment and MLflow maintainer: @harupy

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for updating!

@amyeroberts
Copy link
Collaborator

@seanswyi For the failing tests, these are unrelated and a known issue happening on our CI. We're currently working on a fix for it and will let you know asap once it's merged and you can rebase to get the CI green

@seanswyi
Copy link
Contributor Author

@amyeroberts No worries, thanks for the heads up!

@amyeroberts
Copy link
Collaborator

@seanswyi The failing tests should now be resolved. Could you try rebasing on main?

@seanswyi
Copy link
Contributor Author

@amyeroberts Just checked that the test pass. Thanks again for the heads up!

@amyeroberts
Copy link
Collaborator

@seanswyi I think something funny has happened with the history in this PR - the diff is now showing changes coming from other unrelated commits, which are also now in the commit history. Did you force push after rebasing? This is necessary as rebasing is effectively rewriting the history

@seanswyi
Copy link
Contributor Author

@amyeroberts Ah, you're right. And no, I don't believe I did a force push, just a regular push. I'll rebase and fix this now.

@amyeroberts
Copy link
Collaborator

Hi @seanswyi, the unrelated diffs are still there. Have you been able to force push?

@seanswyi
Copy link
Contributor Author

Hey @amyeroberts. Yes I did do a force push; the changes I made after my most recent comment refer to the force push. Is this normally supposed to also remove the previously shown messages?

@amyeroberts
Copy link
Collaborator

@seanswyi It shouldn't remove the messages, but it should rewrite the history so that only the commits relating to this branch are shown and on the files tab only their respective diffs. At the moment, there's a diff in 130 files, which aren't related to this PR, and so we cannot merge as-is.

The reason I ask about force pushing is the current history looks like what happens when one rebases but doesn't force push (lots of commits on main and no commit saying x force-pushed the foo branch, most recently from XXX to YYY). I would try rebasing on main and force pushing again.

The previous code was calling the `mlflow.set_tracking_uri` function
regardless of whether or not the environment variable
`MLFLOW_TRACKING_URI` is even set. This led to clashes with the original
MLflow implementation and therefore the logic was changed to only
calling the function when the environment variable is explicitly set.
The previous code did not consider the possibility that the tracking URI
may already be set elsewhere and was therefore (erroneously) overriding
previously set tracking URIs using the environment variable.
@seanswyi
Copy link
Contributor Author

@amyeroberts I made the changes mentioned in a comment from a previous PR (#29032 (comment)) and did a force push. Just to double check, the sequence of commands I'm running is as follows:

git fetch upstream
git rebase upstream/main
git pull
git push --force

Is this the correct? Just wanted to double check because after doing the force push I'm seeing the other commits again.

@amyeroberts
Copy link
Collaborator

@seanswyi Ah, I see what's happening. There shouldn't be a pull step. Depending on your git settings, this will probably create a merge commit, merging the remote branch into the local. This will create a contradiction however, as doing a rebase is effectively rewriting the history of the local branch. The steps are:

git fetch upstream main
git rebase upstream/main
git push -f

@seanswyi
Copy link
Contributor Author

seanswyi commented Mar 1, 2024

@amyeroberts Thanks for letting me know, I wasn't sure what I was doing wrong. I just did another force push without the pull this time. 👍

Copy link
Collaborator

@amyeroberts amyeroberts 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 fixing!

Just a few small nits and we'll be good to merge!

src/transformers/integrations/integration_utils.py Outdated Show resolved Hide resolved
src/transformers/integrations/integration_utils.py Outdated Show resolved Hide resolved
src/transformers/integrations/integration_utils.py Outdated Show resolved Hide resolved
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

seanswyi and others added 3 commits March 1, 2024 19:04
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
"Unset by default" is the correct expression rather than "Default to `None`."

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@amyeroberts amyeroberts merged commit 81220cb into huggingface:main Mar 4, 2024
21 checks passed
damithsenanayake pushed a commit to damithsenanayake/transformers that referenced this pull request Mar 7, 2024
… with original MLflow code. (huggingface#29096)

* Changed logic for setting the tracking URI.

The previous code was calling the `mlflow.set_tracking_uri` function
regardless of whether or not the environment variable
`MLFLOW_TRACKING_URI` is even set. This led to clashes with the original
MLflow implementation and therefore the logic was changed to only
calling the function when the environment variable is explicitly set.

* Check if tracking URI has already been set.

The previous code did not consider the possibility that the tracking URI
may already be set elsewhere and was therefore (erroneously) overriding
previously set tracking URIs using the environment variable.

* Removed redundant parentheses.

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

* Fix docstring to reflect library convention properly.

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

* Fix docstring to reflect library convention properly.

"Unset by default" is the correct expression rather than "Default to `None`."

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

---------

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
itazap pushed a commit that referenced this pull request May 14, 2024
… with original MLflow code. (#29096)

* Changed logic for setting the tracking URI.

The previous code was calling the `mlflow.set_tracking_uri` function
regardless of whether or not the environment variable
`MLFLOW_TRACKING_URI` is even set. This led to clashes with the original
MLflow implementation and therefore the logic was changed to only
calling the function when the environment variable is explicitly set.

* Check if tracking URI has already been set.

The previous code did not consider the possibility that the tracking URI
may already be set elsewhere and was therefore (erroneously) overriding
previously set tracking URIs using the environment variable.

* Removed redundant parentheses.

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

* Fix docstring to reflect library convention properly.

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

* Fix docstring to reflect library convention properly.

"Unset by default" is the correct expression rather than "Default to `None`."

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

---------

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
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.

4 participants