Decoupled checkpoint artifact path from model artifact path

[pete-mcelroy]

What does this PR do?

Addresses the issues described in issue 19394 by making the checkpoint artifact path in the MLFlowLogger class fully customizable by removing the enforced "model/checkpoints" prefix.

As shown in the image below, we can now specify an artifact path for our checkpoints outside the model/ subfolder. This means that when we want to load a model, we do not have to load all checkpoints at the same time.

Fixes #19394

Before submitting

• Was this discussed/agreed via a GitHub issue? (not for typos and docs)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together?
• Did you make sure to update the documentation with your changes? (if necessary)
• Did you write any new necessary tests? (not for typos and docs)
• Did you verify new and existing tests pass locally with your changes?
• Did you list all the breaking changes introduced by this pull request?
• Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

Reviewer checklist

• Is this pull request ready for review? (if not, please submit in draft mode)
• Check that all items from Before submitting are resolved
• Make sure the title is self-explanatory and the description concisely explains the PR
• Add labels and milestones (and optionally projects) to the PR so it can be classified

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81%. Comparing base (5dea36c) to head (9e94053).
Report is 72 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (5dea36c) and HEAD (9e94053). Click for more details.

HEAD has 554 uploads less than BASE

Flag	BASE (5dea36c)	HEAD (9e94053)
cpu	148	21
python3.10	42	6
lightning_fabric	25	0
pytest	87	2
lightning	107	16
python3.9	43	6
python3.11	42	6
gpu	3	2
python3.12	21	3
pytorch2.1	37	12
pytest-full	64	21
pytorch2.3	9	3
pytorch_lightning	19	7
pytorch2.2	9	3
pytorch2.4	9	3

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #20325     +/-   ##
=========================================
- Coverage      88%      81%     -7%     
=========================================
  Files         267      264      -3     
  Lines       23065    23010     -55     
=========================================
- Hits        20277    18606   -1671     
- Misses       2788     4404   +1616     

[le-Greg]

How to get the old behaviour now ? I would like to keep my models stored under /models, now they are logged in the root /, which is a bit messy. I tried artifact_location but it doesn't change anything, and save_dir doesn't work since i am using a tracking uri

[pete-mcelroy]

How to get the old behaviour now ? I would like to keep my models stored under /models, now they are logged in the root /, which is a bit messy. I tried artifact_location but it doesn't change anything, and save_dir doesn't work since i am using a tracking uri

In your ModelCheckpoint callback, if you set the 'filename' parameter equal to 'model', your .ckpt file will be saved inside the 'model' folder.

[joncarter1]

@pete-mcelroy this is not the same as the old behaviour and, like @le-Greg, I also preferred the original nested format.

For example, I believe it is no longer possible to nest checkpoints under a consistent directory with this change, e.g.:

@tchaton can we make this configurable? e.g. with a checkpoint_dir argument to the MLFlowLogger which defines the sub-folder where they get logged? Happy to make MR if that approach sounds reasonable. Will stick with Lightning < 2.5 in the meantime.

Nvm looks like #20538 is about to fix this 🎉