Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support prepend argument in TBLogger constructor #101

Merged
merged 2 commits into from
Jun 7, 2021
Merged

Support prepend argument in TBLogger constructor #101

merged 2 commits into from
Jun 7, 2021

Conversation

ancapdev
Copy link
Contributor

@ancapdev ancapdev commented Jun 7, 2021

I'm working on a project where I run multiple threads producing tensorboard events for the same run (sharing a log directory), each thread producing events for different tags. The time() based file naming mechanism isn't guaranteed to avoid, and in fact leads to, collisions under this use case. To resolve the problem I've added support for the existing create_eventfile() prepend argument to TBLogger().

Appreciate your reviewing and considering this small change. Thanks

@PhilipVinc
Copy link
Member

Hi and thanks for your contribution.

Maybe a better name would be prefix, which is more consistent with what is used in a few cases in the sdtlib.
Can you change it in the TBLogger constructor?

Also, could you add a line to the docstring explaining what it does (mentioning that it's the prefix of the files and not of the folder directory).
If you have the time, also adding a test that checks that it indeed works would be great.

@PhilipVinc
Copy link
Member

Also, For the moment I'm trying to support older (Julia>=1.3) versions so you cannot use the short kwargs syntax

@PhilipVinc
Copy link
Member

Otherwise this seems fine.
If you address the comments above i'll merge and quickly tag a new release.

@ancapdev
Copy link
Contributor Author

ancapdev commented Jun 7, 2021

Sure, sounds good. Do you want it named prefix in create_eventfile() as well? I was just trying to be consistent with that.

@PhilipVinc
Copy link
Member

yes if you can rename it it would be great

@PhilipVinc PhilipVinc merged commit 1988a21 into JuliaLogging:master Jun 7, 2021
@ancapdev ancapdev deleted the prepend branch June 7, 2021 15:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants