TensorBoardLogger sub_dir parameter for grouping logs

[janhenriklambrechts]

What does this PR do?

TLDR: Adds a sub_dir parameter for the TensorBoardLogger useful for grouping multiple runs in one folder as per #6019.

Although in #6019 I advocated for a general sub_dir arg for all logger instances to I eventually added it as a TensorBoardLogger-only parameter. Therefore, this PR only solves the "group multiple tensorboard logs into one"-aspect and not the "keep different logger-type logs in different folders"-aspect. It would still be cool to arrive at the solution described in #6019 - yet that will take a bigger overhaul of the logging module (f.e. adding version parameter to all loggers).

Before submitting

• Was this discussed/approved 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 update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

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

• 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

Did you have fun?

Make sure you had fun coding 🙃

[pep8speaks]

Hello @janhenriklambrechts! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-18 01:26:07 UTC

[codecov]

Codecov Report

Merging #6195 (3fb1bec) into master (20f6337) will decrease coverage by 5%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #6195    +/-   ##
=======================================
- Coverage      92%     88%    -5%     
=======================================
  Files         197     197            
  Lines       12878   12886     +8     
=======================================
- Hits        11899   11320   -579     
- Misses        979    1566   +587     

[s-rog]

@janhenriklambrechts I think that is failing from either a missing or extra / at the end of the path

[s-rog]

@janhenriklambrechts
From the docs of os.path.join:

... will only end in a separator if the last part is empty

Example:

os.path.join("1", "2", "3")
'1/2/3'
os.path.join("1", "2", "")
'1/2/'

That's why it's outputting the extra separator in tests.

[janhenriklambrechts]

@s-rog thanks! Using this info to hotfix that test now :)

[s-rog]

Oh I already went ahead and fixed it! also, for the test can you actually pass in env vars?

[janhenriklambrechts]

@s-rog ah rip, just saw your message that you went ahead and fixed it - should I revert these commits?

Also, what do you exactly mean with env_vars - I did add a test for handling ~ and $ correctly. What is the desired behaviour for env vars?

[s-rog]

Yeah, I think it's best to not change the old test and have consistent separator usage regardless of sub dir usage.

lmk if you run into git issues and I'll see what I can do from my end.

[janhenriklambrechts]

@s-rog reverted - thanks for your help!
This only leaves this:

what do you exactly mean with env_vars - I did add a test for handling ~ and $ correctly. What is the desired behaviour for env vars?

[s-rog]

@janhenriklambrechts I went ahead and added it, should be good to merge once test pass!

[janhenriklambrechts]

@s-rog cool thanks - I watch and learn 😄

[s-rog]

@justusschock Your suggestion should be addressed if you wanna take another look

Edit:
I just realized that perhaps the env var can be mocked... though not sure if expandvars would pick it up