Avoid phantom metadata in `upload_artifact` for missing sources by INo-xious · Pull Request #6880 · optuna/optuna · GitHub
Skip to content

Avoid phantom metadata in upload_artifact for missing sources - #6880

Merged
c-bata merged 2 commits into
optuna:masterfrom
INo-xious:fix/artifact-upload-missing-source
Oct 20, 2026
Merged

c-bata merged 2 commits into
optuna:masterfrom
INo-xious:fix/artifact-upload-missing-source

Conversation

@INo-xious

Copy link
Copy Markdown
Contributor

Motivation

upload_artifact currently stores artifact metadata before opening the source. A missing file raises FileNotFoundError but leaves an artifact record with no uploaded content.

Description of the changes

Open the source before registering metadata while keeping metadata registration before the backend write. Keep both operations in the existing file context so the source closes on success or failure.

Add regressions for Study, Trial and FrozenTrial using in-memory, SQLite and journal storage. They verify unchanged pre-existing metadata after a missing-source failure and a subsequent readable upload.

Validation: the 9 regressions fail against the base implementation and pass with the fix; 26 local artifact tests, repository-wide Ruff checks and formatting, and mypy pass.

Checklist

  • I used an LLM while working on this PR.

@not522

not522 commented Oct 2, 2026

Copy link
Copy Markdown
Member

@not522 not522 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@not522 not522 removed their assignment Oct 6, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has not seen any recent activity.

@github-actions github-actions Bot added the stale Exempt from stale bot labeling. label Oct 13, 2026
@c-bata c-bata removed the stale Exempt from stale bot labeling. label Oct 14, 2026

@c-bata c-bata left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for your pull request and sorry for my delayed review. I left one request for change.

Comment thread optuna/artifacts/_upload.py Outdated
Comment on lines +111 to +117

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we register the metadata after artifact_store.write succeeds? This would also avoid leaving metadata behind if the artifact store write fails.

A failed metadata update could leave an unreferenced artifact in the store, but the artifact would not appear as uploaded.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I’ll move metadata registration after artifact_store.write() succeeds.

@c-bata c-bata added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Oct 20, 2026
@c-bata c-bata added this to the v5.1.0 milestone Oct 20, 2026

@c-bata c-bata left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@c-bata
c-bata merged commit 0f1cc4c into optuna:master Oct 20, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants