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

define public API for torch.nn.utils #111026

Closed
wants to merge 7 commits into from

Conversation

ringohoffman
Copy link
Contributor

Adding modules imported here and the following functions to the __all__:

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 11, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/111026

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 146d78c with merge base 95ff51d (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@ringohoffman
Copy link
Contributor Author

clip_grad_norm (deprecated), fuse_conv_bn_eval, fuse_conv_bn_weights, and convert_conv2d_weight_memory_format are all imported here but do not have docs pages.

@vadimkantorov
Copy link
Contributor

Btw vector_to_params / params_to_vector seems related to flatten_parameters concept ...

Copy link
Contributor

@mikaylagawarecki mikaylagawarecki left a comment

Choose a reason for hiding this comment

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

You can expose the documentation for the functions you mentioned that are missing from the documentation over here

Those functions you mentioned that are failing the public bindings test should still be added to the __all__

@mikaylagawarecki mikaylagawarecki added release notes: nn release notes category topic: improvements topic category labels Oct 11, 2023
@ringohoffman
Copy link
Contributor Author

I added the other 2 functions from fusion since it seemed strange that they would be omitted. I also tried to split up nn.rst into more logical sections so that the functions in the same submodule would be kept and documented together. LMK how this looks.

Notes can have more than one paragraph. Successive paragraphs must indent to the same level as the rest of the note: https://sublime-and-sphinx-guide.readthedocs.io/en/latest/notes_warnings.html#notes
Copy link
Contributor

@mikaylagawarecki mikaylagawarecki left a comment

Choose a reason for hiding this comment

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

For future reference, you can check out the docs preview from your PR under

📄 Preview Python docs built from this PR

in the pytorch-bot comment above.

It renders nicely to me, it would be awesome if you could add short docstrings for fuse_conv_bn_eval
fuse_conv_bn_weights
fuse_linear_bn_eval
fuse_linear_bn_weights

as well since they come up as blank in the table
Screenshot 2023-10-11 at 5 49 21 PM

@ringohoffman
Copy link
Contributor Author

@mikaylagawarecki I gave it my best effort! Please LMK what you think of my changes.

@ringohoffman
Copy link
Contributor Author

ringohoffman commented Oct 12, 2023

fuse_conv_bn_weights & fuse_linear_bn_weights are written such that they expect that bn_rm and bn_rv are not None, but there are a few places where these functions are called without checking that they are not None. I think the easiest thing to do will be to assert that they are not None in all these places these functions are called since they will certainly raise TypeErrors if they are None anyway.

Copy link
Contributor

@mikaylagawarecki mikaylagawarecki left a comment

Choose a reason for hiding this comment

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

This is some much appreciated cleanup! Thanks a lot for your effort :)

@mikaylagawarecki
Copy link
Contributor

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 12, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

sonphantrung added a commit to sonphantrung/Ilaria-RVC-Mainline-another that referenced this pull request Apr 15, 2024
As pytorch/pytorch#111026 got merged, the workaround became redundant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: nn release notes category topic: improvements topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants