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

[seanswyi]

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

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline,
  Pull Request section?
• Was this discussed/approved via a Github issue or the forum? Please add a link
  to it if that's the case. https://github.com/omnious/model-classifier/blob/885efa563cba97e58ed5c5874a2c9d0f32daded2/tests/unit/test_model.py#L1185
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

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

[amyeroberts]

LGTM - thanks for updating!

[amyeroberts]

@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]

@amyeroberts No worries, thanks for the heads up!

[amyeroberts]

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

[seanswyi]

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

[amyeroberts]

@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]

@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]

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

[seanswyi]

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]

@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.

[seanswyi]

@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]

@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]

@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. 👍

[amyeroberts]

Thanks for fixing!

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

[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.