Skip to content

Conversation

peterbell10
Copy link
Collaborator

@peterbell10 peterbell10 commented Jul 30, 2022

Stack from ghstack (oldest at bottom):

Ref #82518

Starting small to minimize merge conflicts, this moves the top-level
class definitions and some helper functions into the opinfos folder.
It also brings common_methods_invocations.py to just below 1MB.

Ref #82518

Starting small to minimize merge conflicts, this moves the top-level
class definitions and some helper functions into the `opinfos` folder.
It also brings `common_methods_invocations.py` to just below 1MB.

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 30, 2022

🔗 Helpful links

✅ No Failures (2 Pending)

As of commit 87d83ac (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).

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

Click here to manually regenerate this comment.

Ref #82518

Starting small to minimize merge conflicts, this moves the top-level
class definitions and some helper functions into the `opinfos` folder.
It also brings `common_methods_invocations.py` to just below 1MB.

[ghstack-poisoned]
peterbell10 added a commit that referenced this pull request Jul 31, 2022
Ref #82518

Starting small to minimize merge conflicts, this moves the top-level
class definitions and some helper functions into the `opinfos` folder.
It also brings `common_methods_invocations.py` to just below 1MB.

ghstack-source-id: 32963df
Pull Request resolved: #82540
@peterbell10 peterbell10 requested a review from albanD August 3, 2022 15:11
@albanD
Copy link
Collaborator

albanD commented Aug 3, 2022

I'll wait for the discussion on the issue to lead to a consensus before working towards merging this right?

@peterbell10
Copy link
Collaborator Author

Personally I think this should be merged ASAP. It brings common_methods_invocations.py under 1MB which is the main problem atm and this change doesn't depend on how we group the op_db definitions into different files.

Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Ok, the change sounds ok but I would have a couple questions:

  • Why opinfoS (with an S)? This folder does not contain a bunch of opinfos, it is (for now only) the core logic.
  • Where do we plan to put the splitted OpInfo definitions in this new architecture?

@peterbell10
Copy link
Collaborator Author

Where do we plan to put the splitted OpInfo definitions in this new architecture?

My thinking was that we would (eventually) move all the definitions somewhere inside this opinfos folder. File structure TBD.

Why opinfoS (with an S)?

Assuming all the opinfos are defined in the folder it makes more sense. I don't mind dropping it though.

@albanD
Copy link
Collaborator

albanD commented Aug 3, 2022

What about a final structure like?

testing/
  _internal/
    opinfo/
      core.py
      utils.py    
      definitions/
        linalg.py
        fft.py
        nn.py
        ....
  • We use utils.py and not helper.py within the codebase in general
  • Have a subfolder with the definitions to make greping the definitions only simpler.
  • Is the __init__.py actually needed here?

Ref #82518

Starting small to minimize merge conflicts, this moves the top-level
class definitions and some helper functions into the `opinfos` folder.
It also brings `common_methods_invocations.py` to just below 1MB.

[ghstack-poisoned]
peterbell10 added a commit that referenced this pull request Aug 3, 2022
Ref #82518

Starting small to minimize merge conflicts, this moves the top-level
class definitions and some helper functions into the `opinfos` folder.
It also brings `common_methods_invocations.py` to just below 1MB.

ghstack-source-id: 72b42df
Pull Request resolved: #82540
@peterbell10
Copy link
Collaborator Author

peterbell10 commented Aug 3, 2022

We use utils.py and not helper.py within the codebase in general

Sure, I just renamed the existing opinfo_helper.py.

Have a subfolder with the definitions to make greping the definitions only simpler.

Makes sense to me. Though I don't have any strong feelings either way.

Is the __init__.py actually needed here?

AFAIK it's always recommended to include an __init__.py file, even if it's empty. See https://stackoverflow.com/a/48804718/2556706

Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

SGTM!

@albanD
Copy link
Collaborator

albanD commented Aug 3, 2022

Not sure when is the best time to land this to avoid land races...

Ref #82518

Starting small to minimize merge conflicts, this moves the top-level
class definitions and some helper functions into the `opinfos` folder.
It also brings `common_methods_invocations.py` to just below 1MB.

[ghstack-poisoned]
@albanD
Copy link
Collaborator

albanD commented Aug 5, 2022

I'm merging this to avoid any merge conflict if we wait too long.

@albanD
Copy link
Collaborator

albanD commented Aug 5, 2022

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a merge job. Check the current status here

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

Hey @peterbell10.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@peterbell10 peterbell10 added module: testing Issues related to the torch.testing module (not tests) topic: not user facing topic category labels Aug 5, 2022
facebook-github-bot pushed a commit that referenced this pull request Aug 7, 2022
Summary:
Ref #82518

Starting small to minimize merge conflicts, this moves the top-level
class definitions and some helper functions into the `opinfos` folder.
It also brings `common_methods_invocations.py` to just below 1MB.

Pull Request resolved: #82540
Approved by: https://github.com/albanD

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/4d405517e419e08d697a79a37ea7c07effed4e39

Reviewed By: kit1980

Differential Revision: D38478693

fbshipit-source-id: df2afd9cefc43b5a5f8b2afbfd84b0faf37e9d62
@facebook-github-bot facebook-github-bot deleted the gh/peterbell10/369/head branch August 9, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged module: testing Issues related to the torch.testing module (not tests) open source topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants