[DataPipe] Fixing `map` function signature validation by NivekT · Pull Request #84279 · pytorch/pytorch

NivekT · 2022-08-30T16:27:07Z

Stack from ghstack (oldest at bottom):

-> [DataPipe] Fixing map function signature validation #84279

As @pmeier points out, #80267 introduces a bug where an exception is thrown when a built-in function (or a function implemented in C) is used with .map because inspect.signature(fn) cannot find the function's signature.

This PR skips over a function when its signature cannot be found. I believe this case is rare, and if the fn is truly incompatible with the usage of input_col/output_col, an exception will be raised at run time such that users will be able to examine what is wrong.

[ghstack-poisoned]

facebook-github-bot · 2022-08-30T16:27:13Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/84279
✖️ Python docs build was skipped
✖️ C++ docs build was skipped
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

As of commit 161eca9 (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.

ghstack-source-id: 5742ef4 Pull Request resolved: #84279

pmeier

Thanks Kevin for the quick fix. LGTM, if CI is green.

NivekT · 2022-08-30T23:00:33Z

Build failure unrelated (the master branch will be fixed soon). Will land tomorrow.

NivekT · 2022-08-31T15:42:54Z

@pytorchbot rebase

pytorchmergebot · 2022-08-31T15:44:48Z

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

As pmeier [points out](#80267 (comment)), #80267 introduces a bug where an exception is thrown when a built-in function (or a function implemented in C) is used with `.map` because `inspect.signature(fn)` cannot find the function's signature. This PR skips over a function when its signature cannot be found. I believe this case is rare, and if the `fn` is truly incompatible with the usage of `input_col`/`output_col`, an exception will be raised at run time such that users will be able to examine what is wrong. [ghstack-poisoned]

pytorchmergebot · 2022-08-31T15:45:03Z

Successfully rebased gh/nivekt/55/orig onto refs/remotes/origin/master, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/84279)

NivekT · 2022-08-31T15:54:29Z

@pytorchbot merge

pytorchmergebot · 2022-08-31T15:55:53Z

Merge failed

Reason: Matched rule Core Maintainers, but PR #84279 was not reviewed yet by any of: ezyang, gchanan, soumith, dzhulgakov

Details for Dev Infra team

Raised by workflow job

NivekT · 2022-08-31T16:01:20Z

@pytorchbot merge

janeyx99 · 2022-08-31T16:01:37Z

Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea.

pytorchmergebot · 2022-08-31T16:02:49Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered without a flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

… to allow more appropriate messages to be raised first" Changing the ordering in merge rules to allow more appropriate messages to be raised first. Context: [#84279](#84279 (comment)) janeyx99: "Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea." [ghstack-poisoned]

…ppropriate messages to be raised first" Changing the ordering in merge rules to allow more appropriate messages to be raised first. Context: [#84279](#84279 (comment)) janeyx99: "Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea." [ghstack-poisoned]

pytorchmergebot · 2022-08-31T16:27:57Z

Merge failed

Reason: View failures on hud. Refusing to merge as mandatory check(s) pull failed for rule superuser.

Details for Dev Infra team

Raised by workflow job

NivekT · 2022-08-31T17:16:26Z

@pytorchbot rebase

pytorchmergebot · 2022-08-31T17:18:00Z

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

As pmeier [points out](#80267 (comment)), #80267 introduces a bug where an exception is thrown when a built-in function (or a function implemented in C) is used with `.map` because `inspect.signature(fn)` cannot find the function's signature. This PR skips over a function when its signature cannot be found. I believe this case is rare, and if the `fn` is truly incompatible with the usage of `input_col`/`output_col`, an exception will be raised at run time such that users will be able to examine what is wrong. [ghstack-poisoned]

pytorchmergebot · 2022-08-31T17:18:14Z

Successfully rebased gh/nivekt/55/orig onto refs/remotes/origin/master, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/84279)

ghstack-source-id: 7d75fcc Pull Request resolved: #84279

NivekT · 2022-08-31T17:22:58Z

@pytorchbot merge

pytorchmergebot · 2022-08-31T17:24:19Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered without a flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

… to allow more appropriate messages to be raised first" Changing the ordering in merge rules to allow more appropriate messages to be raised first. Context: [#84279](#84279 (comment)) janeyx99: "Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea." [ghstack-poisoned]

…ppropriate messages to be raised first" Changing the ordering in merge rules to allow more appropriate messages to be raised first. Context: [#84279](#84279 (comment)) janeyx99: "Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea." [ghstack-poisoned]

… to allow more appropriate messages to be raised first" Changing the ordering in merge rules to allow more appropriate messages to be raised first. Context: [#84279](#84279 (comment)) janeyx99: "Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea." [ghstack-poisoned]

…ppropriate messages to be raised first" Changing the ordering in merge rules to allow more appropriate messages to be raised first. Context: [#84279](#84279 (comment)) janeyx99: "Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea." [ghstack-poisoned]

… to allow more appropriate messages to be raised first" Changing the ordering in merge rules to allow more appropriate messages to be raised first. Context: [#84279](#84279 (comment)) janeyx99: "Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea." [ghstack-poisoned]

…ppropriate messages to be raised first" Changing the ordering in merge rules to allow more appropriate messages to be raised first. Context: [#84279](#84279 (comment)) janeyx99: "Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea." [ghstack-poisoned]

@janeyx99

…messages to be raised first (#84359) Changing the ordering in merge rules to allow more appropriate messages to be raised first. Context: [#84279](#84279 (comment)) @janeyx99: "Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea." Pull Request resolved: #84359 Approved by: https://github.com/janeyx99, https://github.com/ZainRizvi, https://github.com/malfet

Summary: As pmeier [points out](#80267 (comment)), #80267 introduces a bug where an exception is thrown when a built-in function (or a function implemented in C) is used with `.map` because `inspect.signature(fn)` cannot find the function's signature. This PR skips over a function when its signature cannot be found. I believe this case is rare, and if the `fn` is truly incompatible with the usage of `input_col`/`output_col`, an exception will be raised at run time such that users will be able to examine what is wrong. Pull Request resolved: #84279 Approved by: https://github.com/pmeier, https://github.com/janeyx99 Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/cfb9d0d23314fd28be118b6ca280ded55364e71c Reviewed By: mehtanirav Differential Revision: D39213537 Pulled By: NivekT fbshipit-source-id: 58d176edb3877b0ed0ee74ef4b3a3e61950f7111

…messages to be raised first (#84359) (#84359) Summary: Changing the ordering in merge rules to allow more appropriate messages to be raised first. Context: [#84279](#84279 (comment)) janeyx99: "Approving to unblock, but modifying the merge rules to move the Core maintainers rule to last would be a good idea." Pull Request resolved: #84359 Approved by: https://github.com/janeyx99, https://github.com/ZainRizvi, https://github.com/malfet Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/d648375f13b4a4efd4cd35247098679fce5d4bcd Reviewed By: mehtanirav Differential Revision: D39214439 Pulled By: NivekT fbshipit-source-id: 0fbff5b4aacb9365bdced9d7bbe28bd7c7815a55

[DataPipe] Fixing function sigature validation

5f40b1c

[ghstack-poisoned]

pytorch-bot bot added the release notes: dataloader release notes category label Aug 30, 2022

facebook-github-bot added the cla signed label Aug 30, 2022

NivekT added a commit that referenced this pull request Aug 30, 2022

[DataPipe] Fixing function sigature validation

ef0030c

ghstack-source-id: 5742ef4 Pull Request resolved: #84279

NivekT added the topic: bug fixes topic category label Aug 30, 2022

NivekT requested review from VitalyFedyunin, ejguan and pmeier and removed request for VitalyFedyunin and ejguan August 30, 2022 16:27

NivekT changed the title ~~[DataPipe] Fixing function sigature validation~~ [DataPipe] Fixing map function sigature validation Aug 30, 2022

NivekT requested a review from ejguan August 30, 2022 16:30

pmeier approved these changes Aug 30, 2022

View reviewed changes

NivekT changed the title ~~[DataPipe] Fixing map function sigature validation~~ [DataPipe] Fixing map function signature validation Aug 30, 2022

janeyx99 approved these changes Aug 31, 2022

View reviewed changes

NivekT mentioned this pull request Aug 31, 2022

[GHF] Changing the ordering in merge rules to allow more appropriate messages to be raised first #84359

Closed

pytorchmergebot pushed a commit that referenced this pull request Aug 31, 2022

[DataPipe] Fixing function sigature validation

0add007

ghstack-source-id: 7d75fcc Pull Request resolved: #84279

pytorchmergebot added the Merged label Aug 31, 2022

pytorchmergebot closed this in cfb9d0d Aug 31, 2022

facebook-github-bot deleted the gh/nivekt/55/head branch September 4, 2022 14:19

Conversation

NivekT commented Aug 30, 2022 • edited by pytorchmergebot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

pmeier left a comment

Choose a reason for hiding this comment

Uh oh!

NivekT commented Aug 30, 2022

Uh oh!

NivekT commented Aug 31, 2022

Uh oh!

pytorchmergebot commented Aug 31, 2022

Uh oh!

pytorchmergebot commented Aug 31, 2022

Uh oh!

NivekT commented Aug 31, 2022

Uh oh!

pytorchmergebot commented Aug 31, 2022

Merge failed

Uh oh!

NivekT commented Aug 31, 2022

Uh oh!

janeyx99 commented Aug 31, 2022

Uh oh!

pytorchmergebot commented Aug 31, 2022

Uh oh!

pytorchmergebot commented Aug 31, 2022

Merge failed

Uh oh!

NivekT commented Aug 31, 2022

Uh oh!

pytorchmergebot commented Aug 31, 2022

Uh oh!

pytorchmergebot commented Aug 31, 2022

Uh oh!

NivekT commented Aug 31, 2022

Uh oh!

pytorchmergebot commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NivekT commented Aug 30, 2022 •

edited by pytorchmergebot

Loading

facebook-github-bot commented Aug 30, 2022 •

edited

Loading