Skip to content

Conversation

@isuruf
Copy link
Collaborator

@isuruf isuruf commented Oct 27, 2023

Fixes an issue that came up in #112030

cc @mruberry @ZainRizvi

@isuruf isuruf requested a review from lezcano October 27, 2023 13:10
@pytorch-bot pytorch-bot bot added the release notes: foreach_frontend release notes category label Oct 27, 2023
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 27, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit dee8896 with merge base 28ebe5d (image):
💚 Looks good so far! There are no failures yet. 💚

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

@isuruf
Copy link
Collaborator Author

isuruf commented Oct 27, 2023

@pytorchbot label "module: tests"

@pytorch-bot pytorch-bot bot added the module: tests Issues related to tests (not the torch.testing module) label Oct 27, 2023
Comment on lines 8567 to 8570
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, *args, **kwargs):
self.disable_fastpath = kwargs.pop("disable_fastpath", None)
super().__init__(*args, **kwargs)
self.ref_args = self.args
def __init__(self, *args, **kwargs, disable_fastpath=None):
super().__init__(*args, **kwargs)
self.disable_fastpath = disable_fastpath

Copy link
Collaborator

Choose a reason for hiding this comment

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

Youc ould even assert not kwargs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that we are cleaning things, could you remove this ref_args, and instead use self.args from the parent class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +150 to +151
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now all this is not necessary, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is still necessary because of "values" kwarg which I still don't understand how it's used. I want to clean that up in a separate PR, but I don't understand its usage.

@isuruf isuruf force-pushed the fastpath branch 2 times, most recently from 247021d to eaa74f2 Compare October 27, 2023 15:23
@albanD albanD added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 27, 2023
Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

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

It'd be very nice if we could remove the values kwarg and the ref_args altogether in a follow-up PR

@lezcano
Copy link
Collaborator

lezcano commented Oct 27, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 27, 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

xuhancn pushed a commit to xuhancn/pytorch that referenced this pull request Nov 7, 2023
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
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 module: tests Issues related to tests (not the torch.testing module) open source release notes: foreach_frontend release notes category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants