Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

Add more logging + ability to push to more downstream models #40

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

EntilZha
Copy link
Contributor

This PR adds:

  • repr functions for some classes
  • Rich based logging: https://rich.readthedocs.io/en/stable/introduction.html
  • Extra parameter additional_downstream_models to agent.train that can be used to push to more than one downstream model (e.g., if there are multiple parallel loops, push to the model that each loop is using).

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 20, 2022
@bcui19
Copy link
Contributor

bcui19 commented Apr 21, 2022

I was wondering why switch to rich based logging vs normal python logging? Also if we're changing to rich based logging, would you want to propagate it through all of the 'examples' as well? @xiaomengy not sure if you have any thoughts on how we should do logging in the code base.

@EntilZha
Copy link
Contributor Author

The main reasons I swapped is that in the past (and now too), (1) I've had trouble getting python to actually output what I need (configure log level correctly) + (2) rich does a nice job of colonizing input. I'm not married to it, but I do like it quite a bit :).

I'd also be curious about how to do the downstream models, adding it how I did isn't exactly super clean, but I didn't really see another way. You need access to the extra downstream models in inside the train call, but don't really want them to be passed in I think, hence the optional arg.

@bcui19
Copy link
Contributor

bcui19 commented Apr 22, 2022

re: downstream models yeah I think it might be better if we made it a class variable. If we need to wait for the other inference servers to connect, then we could make add/set functions to downstream models (also maybe remove if downstream model nodes go offline).

@bcui19
Copy link
Contributor

bcui19 commented Apr 28, 2022

LGTM!

Copy link
Contributor

@xiaomengy xiaomengy left a comment

Choose a reason for hiding this comment

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

Please try to format the code by yapf. Thanks.

@@ -243,7 +266,7 @@ def __call__(self, index: int):
self._optimizer, self._batch_size, self._grad_clip,
self._multi_step, self._gamma,
self._learning_starts, self._sync_every_n_steps,
self._push_every_n_steps, self._collate_fn)
self._push_every_n_steps, self._collate_fn, additional_models_to_update=self._additional_models_to_update)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to format this file by yapf. We have the line limit of 80 characters.

@bcui19 bcui19 merged commit ab02952 into facebookresearch:main Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants