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.

Sign up for GitHub

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

Jump to bottom

[Feature] Add all implemented loggers to the init of loggers #402

Merged
merged 5 commits into from
Sep 9, 2022
Merged

[Feature] Add all implemented loggers to the init of loggers #402

merged 5 commits into from
Sep 9, 2022

Conversation

flinder
Copy link

@flinder flinder commented Sep 8, 2022 •
edited
Loading

Description

Adds all implemented loggers to trainers.logger.__init__.py

Motivation and Context

close #349

Types of changes

What types of changes does your code introduce? Remove all that do not apply:

  • Bug fix (non-breaking change which fixes an issue)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

  • [x ] I have read the CONTRIBUTION guide (required)
  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.

@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 Sep 8, 2022
@vmoens vmoens changed the title Add all implemented loggers to the init of loggers [Feature] Add all implemented loggers to the init of loggers Sep 8, 2022
@vmoens
Copy link
Contributor

vmoens commented Sep 8, 2022

Seems like the tests are failing because of new gym version, let me investigate :)

@vmoens vmoens added the bug Something isn't working label Sep 8, 2022
vmoens
vmoens approved these changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@vmoens vmoens left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this
Let's just wait until the CI is fixed and tests pass, then we'll land this :)

@vmoens
Copy link
Contributor

vmoens commented Sep 9, 2022

LGTM, thanks for this Let's just wait until the CI is fixed and tests pass, then we'll land this :)

You can merge the PR I created, that should fix the tests!

@flinder
Copy link
Author

flinder commented Sep 9, 2022

LGTM, thanks for this Let's just wait until the CI is fixed and tests pass, then we'll land this :)

You can merge the PR I created, that should fix the tests!

Thanks for fixing this so quickly. I don't have write access though, so can't merge PRs

@vmoens
Copy link
Contributor

vmoens commented Sep 9, 2022 •
edited
Loading

No I meant on your fork there's a PR from myself.
Otherwise you can just merge main from torchrl onto your branch

Merge pull request #1 from facebookresearch/main
51806e1 
[BugFix] Temporarily fix gym to 0.25.1 to fix CI (pytorch#411)
@flinder
Copy link
Author

flinder commented Sep 9, 2022

No I meant on your fork there's a PR from myself. Otherwise you can just merge main from torchrl onto your branch

oh sorry, somehow missed that one. I merged it

@vmoens
Copy link
Contributor

vmoens commented Sep 9, 2022

Fantastic, thanks to your contribution we've found a bug in the tests.
In test_trainer.py we were importing TensorboardLogger from from torchrl.trainers.loggers which resulted in an error => the _has_tb was always False and all the tests that requires TensorboardLogger were silently ignored.

I would suggest to amend your PR with the two following changes:
(1) Even with your edit I think it would make more sense to import TensorboardLogger from its original file and not torchrl.trainers.loggers in test_trainer.py
(2) We can fix this bug by simply adding args.collector_devices = ["cpu"]

vmoens
vmoens approved these changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@vmoens vmoens left a comment

Choose a reason for hiding this comment

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

LGreatTM!
Don't worry about the failing OSX test, we just need to wait for a millisecond between two operations to make sure that the tensorboard thread has done its job...
It's a flacky test that needs to be fixed.

@vmoens vmoens merged commit b8c0cde into pytorch:main Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vmoens vmoens vmoens approved these changes

Assignees
No one assigned
Labels
bug Something isn't working 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Loggers registration
3 participants
@flinder @vmoens @facebook-github-bot