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

Solved minor bug with MLFlow logger #16418

Merged
merged 6 commits into from
Jan 20, 2023
Merged

Solved minor bug with MLFlow logger #16418

merged 6 commits into from
Jan 20, 2023

Conversation

BrianPulfer
Copy link
Contributor

@BrianPulfer BrianPulfer commented Jan 18, 2023

Resolves #16411 the minor issue #16411 (typo) from

params_list.append(Param(key=v, value=v))

to

params_list.append(Param(key=k, value=v))

@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Jan 18, 2023
@Borda Borda added this to the v1.9.x milestone Jan 18, 2023
@awaelchli awaelchli self-assigned this Jan 18, 2023
@Borda Borda changed the title Solved minor bug (Issue #16411). Solved minor bug with MLFlow logger Jan 18, 2023
@Borda Borda enabled auto-merge (squash) January 18, 2023 14:16
@senarvi
Copy link
Contributor

senarvi commented Jan 19, 2023

For me this fixes MLflow logging.

But now that I look at the code, what do you think of replacing the entire loop with this:

params_list = [Param(key=k, value=str(v)[:250]) for k, v in params.items()]

i.e. instead of not logging long string values, we would log the first 250 characters?

@riklopfer riklopfer mentioned this pull request Jan 19, 2023
12 tasks
@mergify mergify bot added the ready PRs ready to be merged label Jan 19, 2023
@awaelchli awaelchli added bug Something isn't working logger: mlflow labels Jan 19, 2023
@awaelchli
Copy link
Contributor

@senarvi That sounds good to me. Feel free to send a PR with this improvement. Fewer warnings are always better if it can be avoided :)

src/lightning_fabric/CHANGELOG.md Outdated Show resolved Hide resolved
@schmidt-jake
Copy link
Contributor

schmidt-jake commented Jan 19, 2023

@senarvi thanks for catching this! LGTM.

As for your suggestion, two thoughts:

  1. Truncating keys to 250 characters may result in duplicate keys — thoughts on how to handle this?
  2. MLFlow version 1.28 increased the max param length to 500 — would be nice to take advantage of this

Also — should we have key=str(k) to be safe?

@awaelchli awaelchli added the community This PR is from the community label Jan 19, 2023
@Borda Borda merged commit 6fd914f into Lightning-AI:master Jan 20, 2023
Borda pushed a commit that referenced this pull request Feb 9, 2023
Resolves #16411

(cherry picked from commit 6fd914f)
lantiga pushed a commit that referenced this pull request Feb 10, 2023
Resolves #16411

(cherry picked from commit 6fd914f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working community This PR is from the community logger: mlflow pl Generic label for PyTorch Lightning package ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typo breaks mlflow logger in 1.9.0
6 participants