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 multi-process logger utility for status monitoring #254

Merged
merged 11 commits into from
Feb 3, 2023
Merged

Conversation

jon-tow
Copy link
Collaborator

@jon-tow jon-tow commented Feb 2, 2023

This PR introduces a basic logging.Logger utility for status monitoring from the console with multi-process support. By default, the logger is set to the INFO level but verbosity can be controlled by setting the TRLX_LOG_LEVEL environment variable to one of the standard logging level names. Check the README.md update for more.

@jon-tow jon-tow marked this pull request as ready for review February 2, 2023 16:39
@jon-tow jon-tow changed the title Add logger utility for detailed status monitoring Add multi-process logger utility for status monitoring Feb 2, 2023
Copy link
Collaborator

@maxreciprocate maxreciprocate left a comment

Choose a reason for hiding this comment

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

Thanks Jon this will help with debugging timeouts massively! Left a few tiny nitpicks 🙏

README.md Outdated Show resolved Hide resolved
trlx/orchestrator/ppo_orchestrator.py Outdated Show resolved Hide resolved
trlx/orchestrator/ppo_orchestrator.py Outdated Show resolved Hide resolved
trlx/orchestrator/ppo_orchestrator.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
export TRLX_LOG_LEVEL=WARNING
```

> 💡 Tip: To reduce the amount of logging output, you might find it helpful to change log levels of third-party libraries. For the `transformers` library, try setting `transformers.logging.set_verbosity_error()`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's really handy, they've added some annoying logs in 4.25.1 (especially if you set prompts to [<eos>]). Can this method be used for trlx as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup! I should clarify this - I meant to suggest calling transformers.logging.set_verbosity_error() from a user's trlx script e.g. at the top of examples/ilql_sentiments.py.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Let me know if it's any clearer 😄 )

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I understood, I will also clarify that I meant to ask whether you can currently do trlx.logging.set_verbosity_error()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, not currently. Maybe we should include logging methods in a similar way to transformers. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean since we're already half-way there we might as well indulge, unless it's harder than it seems of course 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😂 will do!

@jon-tow
Copy link
Collaborator Author

jon-tow commented Feb 3, 2023

Update: I've added Hugging Face's logging API (used across their projects datasets, diffusers, transformers)

Copy link
Collaborator

@maxreciprocate maxreciprocate left a comment

Choose a reason for hiding this comment

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

Nice, it looks really pretty especially when training with larger models!

(also I've observed that deepspeed's logger does everything similarly and can be filtered with deepspeed.utils.logger.setLevel(logging.ERROR))

Copy link
Contributor

@Mistobaan Mistobaan left a comment

Choose a reason for hiding this comment

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

  • I would not use all this code for logging.
  • push the rank configurations around the code that access global variables inside the logging configuration. As a rule of thumb the env variables should be access at the start of the process at configuration time and only once.
  • decide on the env variable to use TRLX_LOG_LEVEL vs TRLX_VERBOSITY
  • printing tables in the logs is a nightmare for devops because parsing the output programmatically becomes very hard.


This will suppress `INFO` level messages, but still print `WARNING`, `ERROR`, and `CRITICAL` level messages.

You can also control logging verbosity by setting the `TRLX_VERBOSITY` environment variable to one of the standard logging [level names](https://docs.python.org/3/library/logging.html#logging-levels):
Copy link
Contributor

Choose a reason for hiding this comment

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

is it TRLX_VERBOSITY or TRLX_LOG_LEVEL ?
I kind of like more TRLX_LOG_LEVEL

trlx/utils/logging.py Outdated Show resolved Hide resolved
@jon-tow
Copy link
Collaborator Author

jon-tow commented Feb 3, 2023

@Mistobaan Yes, I agree with your points but the idea was to keep the API consistent with Hugging Face's logging API which is familiar to many. See links from #254 (comment)

If you have strong opinions about not doing this I can revert to the commit prior to introducing this (3034595).

Re tables: They already exist in the current main - feel free to open a separate issue to detail potential issues.

@jon-tow jon-tow merged commit 6427429 into main Feb 3, 2023
@jon-tow jon-tow deleted the log-verbosity branch February 10, 2023 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants