Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add ChainLogger, PyTorchLogger and LookupLogger #19

Merged
merged 19 commits into from
Jan 3, 2023

Conversation

shadeMe
Copy link
Collaborator

@shadeMe shadeMe commented Dec 19, 2022

ChainLogger - Allows multiple loggers to be daisy-chained. The number of links in the chain is hard-coded as confection neither supports second-order resolution of registry functions nor does its parser correctly convert list primitives inside the same. This also interferes with pydantic validation, making the following syntax impossible:

[training.logger]
@loggers = "spacy.ChainLogger.v1"
chain = [ { "@loggers": "spacy.TestLogger.v1", "val": "random"}, { "@loggers": "spacy.ConsoleLogger.v3", "progress_bar": "train"}]

PyTorchLogger - Queries PyTorch stats and passes them to other loggers. We wanted to track CUDA memory statistics during training/distillation, but there was no straight-forward way to pass that information to the Weights and Biases logger, thus giving rise to logger chaining.

LookupLogger - Meant to be used to as a development tool when working with logger interop (wrote it for testing the above two additions).

I still need to add support for tracking arbitrary values in WandbLogger, but that'll come in a separate PR.

@shadeMe shadeMe changed the title Add ChainLogger', PyTorchLogger and LookupLogger` Add ChainLogger, PyTorchLogger and LookupLogger Dec 19, 2022
spacy_loggers/chain.py Outdated Show resolved Hide resolved
spacy_loggers/chain.py Outdated Show resolved Hide resolved
@shadeMe shadeMe added the enhancement New feature or request label Dec 22, 2022
README.md Outdated Show resolved Hide resolved
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Nice!

The loggerX arguments aren't very satisfying as you noted, but I don't see an immediate way around this either.

One thing: we should add at least a few unit tests here. I see that we haven't been in the habit of doing so for spacy-loggers, but we really should. Checking console output etc is not straightforward, but we can at least check initialization from a config, both happy and non-happy path, etc.

README.md Outdated Show resolved Hide resolved
spacy_loggers/lookup.py Outdated Show resolved Hide resolved
@shadeMe
Copy link
Collaborator Author

shadeMe commented Dec 30, 2022

Added a new utility method that will be needed for the upcoming PRs.

spacy_loggers/lookup.py Outdated Show resolved Hide resolved
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Tests are looking good, thanks!

Just had a few more minor nitpicks...

README.md Outdated Show resolved Hide resolved
spacy_loggers/tests/test_chain.py Outdated Show resolved Hide resolved
spacy_loggers/util.py Outdated Show resolved Hide resolved
spacy_loggers/util.py Outdated Show resolved Hide resolved
Clarify error message
Fix `None` check
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Should be good to merge once the tests go green :-)
Nice work!

@svlandeg svlandeg merged commit 5618829 into explosion:main Jan 3, 2023
@shadeMe shadeMe deleted the feature/chain-loggers branch January 3, 2023 10:35
@tpanza tpanza mentioned this pull request Aug 16, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants