KEMBAR78
TST Adds __repr__ and str to module info by thomasjpfan · Pull Request #63737 · pytorch/pytorch · GitHub
Skip to content

TST Adds __repr__ and str to module info #63737

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
Closed
wants to merge 4 commits into from
Closed

TST Adds __repr__ and str to module info #63737

wants to merge 4 commits into from

Conversation

@thomasjpfan
Copy link
Contributor

@thomasjpfan thomasjpfan commented Aug 22, 2021 •
edited by pytorch-probot bot
Loading

Follow up to #61935

This PR adds test_repr to test_modules.

cc @albanD @mruberry @jbschlosser @walterddr

@thomasjpfan thomasjpfan added module: nn Related to torch.nn triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module module: testing Issues related to the torch.testing module (not tests) labels Aug 22, 2021
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 22, 2021 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 04b0694 (more details on the Dr. CI page):

  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See GitHub Actions build linux-bionic-py3.8-gcc9-coverage / test (default, 2, 2, linux.2xlarge) (1/1)

Step: "Unknown" (full log | diagnosis details | 🔁 rerun)

2021-09-01T20:14:25.3404134Z rm: cannot remove ...kins/sccache_error.log': No such file or directory 
2021-09-01T20:14:25.3360619Z ++++ extract_trap_cmd
2021-09-01T20:14:25.3361174Z ++++ printf '%s\n' ''
2021-09-01T20:14:25.3361718Z +++ printf '%s\n' cleanup
2021-09-01T20:14:25.3362430Z ++ trap -- '
2021-09-01T20:14:25.3363051Z cleanup' EXIT
2021-09-01T20:14:25.3365399Z ++ [[ linux-bionic-py3.8-gcc9-coverage-default != *win-* ]]
2021-09-01T20:14:25.3366178Z ++ which sccache
2021-09-01T20:14:25.3373207Z ++ sccache --stop-server
2021-09-01T20:14:25.3397648Z ++ true
2021-09-01T20:14:25.3398076Z ++ rm /var/lib/jenkins/sccache_error.log
2021-09-01T20:14:25.3404134Z rm: cannot remove '/var/lib/jenkins/sccache_error.log': No such file or directory
2021-09-01T20:14:25.3404903Z ++ true
2021-09-01T20:14:25.3405324Z ++ [[ -n '' ]]
2021-09-01T20:14:25.3406037Z ++ [[ linux-bionic-py3.8-gcc9-coverage-default == *rocm* ]]
2021-09-01T20:14:25.3406905Z ++ SCCACHE_ERROR_LOG=/var/lib/jenkins/sccache_error.log
2021-09-01T20:14:25.3407378Z ++ SCCACHE_IDLE_TIMEOUT=1200
2021-09-01T20:14:25.3408265Z ++ RUST_LOG=sccache::server=error
2021-09-01T20:14:25.3408888Z ++ sccache --start-server
2021-09-01T20:14:25.3425061Z sccache: Starting the server...
2021-09-01T20:14:25.3544987Z ++ sccache --zero-stats
2021-09-01T20:14:25.3564911Z Compile requests                      0
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@codecov
Copy link

codecov bot commented Aug 22, 2021 •
edited
Loading

Codecov Report

Merging #63737 (04b0694) into master (421d8f8) will increase coverage by 6.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #63737      +/-   ##
==========================================
+ Coverage   58.96%   65.16%   +6.19%     
==========================================
  Files         705      705              
  Lines       92166    92222      +56     
==========================================
+ Hits        54347    60094    +5747     
+ Misses      37819    32128    -5691

jbschlosser
jbschlosser reviewed Aug 24, 2021
View reviewed changes
Copy link
Contributor

@jbschlosser jbschlosser left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for adding this! Test logic checks out, although this does bring up a tradeoff between test time and test granularity. That is, the __repr__() / str() tests could be merged into another set of tests to avoid the additional overhead of module instantiation. For now, I like this better since it's more modular, and we can look into merging if necessary later on.

jbschlosser
jbschlosser approved these changes Sep 1, 2021
View reviewed changes
Copy link
Contributor

@jbschlosser jbschlosser left a comment

Choose a reason for hiding this comment

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

LGTM! Mind doing a quick rebase?

@facebook-github-bot
Copy link
Contributor

@jbschlosser has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@jbschlosser merged this pull request in 50067c0.

mruberry
mruberry reviewed Sep 2, 2021
View reviewed changes
test/test_modules.py
module_inputs = module_info.module_inputs_func(module_info, device=device, dtype=dtype,
requires_grad=False)
for module_input in module_inputs:
args, kwargs = module_input.constructor_input.args, module_input.constructor_input.kwargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drive-by fyi: one thing to be considerate of in the overall design of ModuleInfo testing is test time. I'm not sure how long running through all the modules and their inputs takes but it might makes sense to glom multiple tests together to avoid having to do it so often

mruberry
mruberry reviewed Sep 2, 2021
View reviewed changes
test/test_modules.py
@modules(module_db)
def test_repr(self, device, dtype, module_info):
# Test module can be represented with repr and str without errors.
module_cls = module_info.module_cls
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this pattern common enough that it's worth abstracting it?

@jbschlosser jbschlosser mentioned this pull request Nov 8, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mruberry mruberry mruberry left review comments

@jbschlosser jbschlosser jbschlosser approved these changes

Assignees

No one assigned

Labels

cla signed Merged module: nn Related to torch.nn module: testing Issues related to the torch.testing module (not tests) open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@thomasjpfan @facebook-github-bot @mruberry @jbschlosser @pytorchbot