Typo breaks mlflow logger in 1.9.0

[markdjwilliams]

Bug description

This line of code appears to be broken in 1.9.0 as it erroneously passes the value of the parameter where it should be providing the name:

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

This should instead likely be:

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

How to reproduce the bug

from collections import OrderedDict
from lightning.pytorch.loggers import MLFlowLogger

params = OrderedDict()
params["a"] = 1

MLFlowLogger().log_hyperparams(params)

Error messages and logs

TypeError: expected string or bytes-like object

Environment

Current environment

#- Lightning Component (e.g. Trainer, LightningModule, LightningApp, LightningWork, LightningFlow):
#- PyTorch Lightning Version (e.g., 1.5.0): 1.9.0
#- Lightning App Version (e.g., 0.5.2):
#- PyTorch Version (e.g., 1.10):
#- Python version (e.g., 3.9):
#- OS (e.g., Linux): Linux
#- CUDA/cuDNN version:
#- GPU models and configuration:
#- How you installed Lightning(`conda`, `pip`, source): source
#- Running environment of LightningApp (e.g. local, cloud): local

More info

No response

cc @Borda

[awaelchli]

@markdjwilliams Great that you spotted it. A PR with the fix would be greatly appreciated if you are interested :)

[markdjwilliams]

Thank you for taking a look. I'm not sure if there's any test coverage here, but there's a chance that this should even be:

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

.... as I can see that mlflow's Param class does seem to expect to be working with only string values:

https://github.com/mlflow/mlflow/blob/2487179a9f239024667946e7d31706a91f2536b8/mlflow/entities/param.py

https://github.com/mlflow/mlflow/blob/2487179a9f239024667946e7d31706a91f2536b8/mlflow/utils/autologging_utils/client.py

[awaelchli]

This code was added in this PR: #15915
Let's ask to double check:
@schmidt-jake Have you used the code with different types for these parameters? Would you mind quickly reviewing the bugfix PR from @BrianPulfer #16418

[Borda]

@BrianPulfer mind commenting here so we can sign the issue to you? 🦦

[schmidt-jake]

Gah, sorry for the silly mistake 😦

Have you used the code with different types for these parameters?

No, but I could add some test cases for this.

Happy to review #16418 if someone with permissions could request it.

[BrianPulfer]

@BrianPulfer mind commenting here so we can sign the issue to you? otter

Yes sure!

[awaelchli]

@schmidt-jake There isn't a "formal" way through GitHub to request your review. If you agree with the change, then we will merge it after adding a test. There was also a question from #16411 (comment) whether the value should be converted to string. But I think that's only necessary for unsupported types. This can be addressed separately if needed.