Skip to content

Fix stale docstring in CustomMLFlowLogger.log_image - #724

Open
AshNicolus wants to merge 3 commits into
mllam:mainfrom
AshNicolus:fix/custom-loggers-docstring
Open

Fix stale docstring in CustomMLFlowLogger.log_image#724
AshNicolus wants to merge 3 commits into
mllam:mainfrom
AshNicolus:fix/custom-loggers-docstring

Conversation

@AshNicolus

@AshNicolus AshNicolus commented Aug 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Describe your changes

log_image's docstring said it raises SystemExit on missing AWS credentials, but that behavior was already replaced by a re-raised NoCredentialsError in #496. The docstring was never updated to match, so it's been silently wrong since then. This just corrects the docstring — no behavior change.

Issue Link

closes #372

Type of change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

  • My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
  • I have performed a self-review of my code
  • For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
  • I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
  • I have updated the README to cover introduced code changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have given the PR a name that clearly describes the change, written in imperative form (context).
  • I have requested a reviewer and an assignee (assignee is responsible for merging). This applies only if you have write access to the repo, otherwise feel free to tag a maintainer to add a reviewer and assignee.

sadamov
sadamov approved these changes Sep 14, 2026
View reviewed changes

@sadamov sadamov left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Checked against main, the code re-raises NoCredentialsError since #496 so the docstring was indeed stale. Will merge after you added a CHANGELOG entry. thanks! @AshNicolus

@sadamov sadamov self-assigned this Sep 14, 2026
AshNicolus added a commit to AshNicolus/neural-lam that referenced this pull request Sep 14, 2026
@AshNicolus
Add CHANGELOG entry for mllam#724
b7aca04
@AshNicolus

AshNicolus commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Added the CHANGELOG entry as requested. Ready to merge!

@sadamov

sadamov commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

there are conflicts with main now after we merged your ther PR. and also please restore the PR template before merge

@sadamov sadamov added bug Something isn't working documentation Improvements or additions to documentation labels Sep 14, 2026
@AshNicolus
Fix stale docstring in CustomMLFlowLogger.log_image
2ec7c76 
sys.exit(1) on NoCredentialsError was replaced with a re-raise in mllam#496,
but the docstring still documented the old SystemExit behavior.

Closes mllam#372
@AshNicolus
Add CHANGELOG entry for mllam#724
8d30076
@AshNicolus
AshNicolus force-pushed the fix/custom-loggers-docstring branch from b7aca04 to 8d30076 Compare September 14, 2026 14:10
@AshNicolus

AshNicolus commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on latest main to resolve conflicts. PR is now conflict-free and ready to merge!

@AshNicolus

AshNicolus commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

All conflicts fully resolved! Added missing #731 CHANGELOG entry. PR is now ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sadamov sadamov sadamov approved these changes

Assignees

@sadamov sadamov

Labels

bug Something isn't working documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Replace sys.exit(1) with exception in custom_loggers.py

2 participants

@AshNicolus @sadamov