Skip to content

Conversation

@Shoumik-Gandre
Copy link

@Shoumik-Gandre Shoumik-Gandre commented Aug 11, 2025

What does this PR do?

Fixes #21035

Before submitting
  • Was this discussed/agreed via a GitHub issue? (not for typos and docs) yes
  • 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

📚 Documentation preview 📚: https://pytorch-lightning--21045.org.readthedocs.build/en/21045/

@github-actions github-actions bot added the fabric lightning.fabric.Fabric label Aug 11, 2025
deependujha
deependujha previously approved these changes Aug 11, 2025
Copy link
Collaborator

@deependujha deependujha left a comment

Choose a reason for hiding this comment

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

Nice work @Shoumik-Gandre.

Could you also update lightning/pytorch/loggers/csv_logs so this behavior is consistent across both Fabric and the Trainer?

@deependujha deependujha dismissed their stale review August 11, 2025 03:37

didn't wanted to approve for now, and wait for test results

@codecov
Copy link

codecov bot commented Aug 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87%. Comparing base (3998b5d) to head (ac69572).
⚠️ Report is 106 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #21045   +/-   ##
=======================================
  Coverage      87%      87%           
=======================================
  Files         269      269           
  Lines       23665    23679   +14     
=======================================
+ Hits        20642    20660   +18     
+ Misses       3023     3019    -4     

@Borda Borda changed the title feature: add add sub_dir to csv logger constructor feature: add add sub_dir to CSV logger constructor Aug 11, 2025
@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Aug 12, 2025
@Shoumik-Gandre
Copy link
Author

Thanks for the review, @deependujha - I've implemented the requested changes.

What changed

  • Add sub_dir to pytorch CSVLogger to match fabric behavior.
  • Ensure log_dir expands ~ and env vars while root_dir preserves the literal input (fixes test_csv_expand_home and env-var cases).
  • Tests updated/added for home and env expansion, and sub_dir.

Why it helps

  • Aligns path semantics across Fabric/PyTorch CSV loggers.
  • Enables cleaner directory layout via sub_dir without breaking existing users.

Safety

  • Backwards compatible (defaults unchanged).
  • All tests pass locally for the touched modules.

@deependujha could you take another look and approve if this matches your expectations?
happy to loop in additional reviewers if needed

Thanks. looking forward to continued contributions!

✅All CI checks are green

@Shoumik-Gandre
Copy link
Author

Hey @williamFalcon hope you’re doing well!
I wanted to check in on this PR. It already has the required number of approvals, but since you're listed as a code owner, I wanted to confirm if you could take a quick look and approve when you get a chance. Your review would help us get it merged smoothly. Thanks a lot! 🙏

@Shoumik-Gandre
Copy link
Author

Hey team. I was wondering if there's anything more I needed to do to get this PR merged?

@Borda
Copy link
Collaborator

Borda commented Aug 14, 2025

Hey team. I was wondering if there's anything more I needed to do to get this PR merged?

you are fine, thank you for your patience, we are waiting Will's approval as he is solo owner of loggers

@Borda Borda changed the title feature: add add sub_dir to CSV logger constructor feature: add sub_dir to CSV logger constructor Aug 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fabric lightning.fabric.Fabric logger: csv logger Related to the Loggers pl Generic label for PyTorch Lightning package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add sub_dir to csv logger constructor

7 participants