Fix: `validate_input_col` for partial functions by BeckerFelix · Pull Request #95067 · pytorch/pytorch

BeckerFelix · 2023-02-17T15:47:26Z

Fixes #95066

Proposed change:

do not call str() on a Callable to determine its name

Reasoning:

Please see #95066 for reasoning and examples

Effect:

The code example given in validate_input_col is impractical for partial functions (with large arguments) #95066 now executes instantly.

If invalid input is provided, the stacktrace now prints nicely as

ValueError: The function foo takes 1 parameters, but 2 are required.

pytorch-bot · 2023-02-17T15:47:28Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ No Failures

As of commit d4b3ed9:
💚 Looks good so far! There are no failures yet. 💚

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

linux-foundation-easycla · 2023-02-17T15:47:29Z

The committers listed above are authorized under a signed CLA.

✅ login: BeckerFelix / name: Felix Becker (cefc6d72686fb66fa1bab9005f541a10297f21ad)

NivekT

LGTM! Thanks for spotting this issue and fixing it. I don't think this should cause any issue for lambda function right?

cc: @ejguan I will cherry-pick this into the release.

BeckerFelix · 2023-02-23T09:45:51Z

LGTM! Thanks for spotting this issue and fixing it. I don't think this should cause any issue for lambda function right?

cc: @ejguan I will cherry-pick this into the release.

thanks for the review.
__name__ is valid on lambda functions, so we should be good.

BeckerFelix · 2023-02-23T09:46:28Z

@pytorchmergebot merge

pytorchmergebot · 2023-02-23T09:48:15Z

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

pytorchmergebot · 2023-02-23T09:48:16Z

Merge failed

Reason: This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again. You can rebase by leaving the following comment on this PR:
@pytorchbot rebase

Details for Dev Infra team

Raised by workflow job

BeckerFelix · 2023-02-23T09:53:55Z

@pytorchbot rebase

pytorch-bot · 2023-02-23T09:53:59Z

You don't have permissions to rebase this PR since you are a first time contributor. If you think this is a mistake, please contact PyTorch Dev Infra.

BeckerFelix · 2023-02-23T10:01:20Z

@pytorchmergebot merge

pytorchmergebot · 2023-02-23T10:03:12Z

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

pytorchmergebot · 2023-02-23T10:03:15Z

Merge failed

Reason: 2 mandatory check(s) failed. The first few are:

pull
Lint

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

Failing merge rule: Core Maintainers

ejguan

Can you pls add tests to test/test_datapipe.py?
You can inject them into test_map_tuple_list_with_col_iterdatapipe and test_map_dict_with_col_iterdatapipe

BeckerFelix · 2023-03-03T15:10:12Z

test_map_tuple_list_with_col_iterdatapipe

Happy to enhance the tests. Can you be more specific of what exactly you would like to see tested, please? In both test_map_tuple_list_with_col_iterdatapipe and test_map_dict_with_col_iterdatapipe map functions, partials, and lambdas get mapped over a datapipe.
I could provide a test which creates a partial with a large input argument and maps this over the datapipe. Similar to the code snippet I provided in #95066. wdyt?

ejguan · 2023-03-03T15:52:45Z

I could provide a test which creates a partial with a large input argument and maps this over the datapipe. Similar to the code snippet I provided in #95066. wdyt?

Sounds good to me.

BeckerFelix · 2023-03-03T17:36:34Z

let me know what you think about d4b3ed9, please

ejguan · 2023-03-03T17:49:01Z

@pytorchbot rebase

pytorchmergebot · 2023-03-03T17:51:07Z

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

pytorchmergebot · 2023-03-03T17:51:11Z

Tried to rebase and push PR #95067, but it was already up to date

ejguan · 2023-03-03T19:26:44Z

@pytorchbot merge

pytorchmergebot · 2023-03-03T19:28:35Z

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

Fixes #95066 #### Proposed change: do not call `str()` on a `Callable` to determine its name #### Reasoning: Please see pytorch/pytorch#95066 for reasoning and examples #### Effect: * The code example given in pytorch/pytorch#95066 now executes instantly. * If invalid input is provided, the stacktrace now prints nicely as ``` ValueError: The function foo takes 1 parameters, but 2 are required. ``` Pull Request resolved: pytorch/pytorch#95067 Approved by: https://github.com/NivekT, https://github.com/ejguan

ejguan · 2023-03-07T18:25:39Z

torch/utils/data/datapipes/utils/common.py

+    if isinstance(fn, functools.partial):
+        fn_name = fn.func.__name__
+    else:
+        fn_name = fn.__name__


This causes lots of problem as not all of callable objects have __name__
I am going to do a forward fix.

Simple example would be a nn.Module

Forward fix the problem introduced in #95067 Not all `Callable` objects have `__name__` implemented. Using `repr` as the backup solution to get function name or reference. Pull Request resolved: #96213 Approved by: https://github.com/NivekT

Forward fix the problem introduced in pytorch/pytorch#95067 Not all `Callable` objects have `__name__` implemented. Using `repr` as the backup solution to get function name or reference. Pull Request resolved: pytorch/pytorch#96213 Approved by: https://github.com/NivekT

Fixes pytorch#95066 #### Proposed change: do not call `str()` on a `Callable` to determine its name #### Reasoning: Please see pytorch#95066 for reasoning and examples #### Effect: * The code example given in pytorch#95066 now executes instantly. * If invalid input is provided, the stacktrace now prints nicely as ``` ValueError: The function foo takes 1 parameters, but 2 are required. ``` Pull Request resolved: pytorch#95067 Approved by: https://github.com/NivekT, https://github.com/ejguan

Forward fix the problem introduced in pytorch#95067 Not all `Callable` objects have `__name__` implemented. Using `repr` as the backup solution to get function name or reference. Pull Request resolved: pytorch#96213 Approved by: https://github.com/NivekT

pytorch-bot bot added the release notes: dataloader release notes category label Feb 17, 2023

pytorchbot added the open source label Feb 17, 2023

bdhirsh assigned ejguan and unassigned ejguan Feb 22, 2023

bdhirsh requested a review from ejguan February 22, 2023 22:29

bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Feb 22, 2023

NivekT approved these changes Feb 23, 2023

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 23, 2023

ejguan requested changes Feb 23, 2023

View reviewed changes

fix: str(fn) is not the way to get the function name

34ae5b8

add test for partial with large arg

d4b3ed9

ejguan added the topic: improvements topic category label Mar 3, 2023

BeckerFelix requested a review from ejguan March 3, 2023 19:21

ejguan approved these changes Mar 3, 2023

View reviewed changes

pytorchmergebot added the Merged label Mar 3, 2023

pytorchmergebot closed this in e6f3e16 Mar 3, 2023

SvenDS9 mentioned this pull request Mar 6, 2023

Add ThreadPoolMapper meta-pytorch/data#1052

Closed

ejguan mentioned this pull request Mar 7, 2023

CI test: check if documentation is complete meta-pytorch/data#1070

Closed

ejguan reviewed Mar 7, 2023

View reviewed changes

ejguan mentioned this pull request Mar 7, 2023

Fix validate_input_col for nn.Module or Callable #96213

Closed

Conversation

BeckerFelix commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change:

Reasoning:

Effect:

Uh oh!

pytorch-bot bot commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ No Failures

Uh oh!

linux-foundation-easycla bot commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NivekT left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeckerFelix commented Feb 23, 2023

Uh oh!

BeckerFelix commented Feb 23, 2023

Uh oh!

pytorchmergebot commented Feb 23, 2023

Merge started

Uh oh!

pytorchmergebot commented Feb 23, 2023

Merge failed

Uh oh!

BeckerFelix commented Feb 23, 2023

Uh oh!

pytorch-bot bot commented Feb 23, 2023

Uh oh!

BeckerFelix commented Feb 23, 2023

Uh oh!

pytorchmergebot commented Feb 23, 2023

Merge started

Uh oh!

pytorchmergebot commented Feb 23, 2023

Merge failed

Uh oh!

ejguan left a comment

Choose a reason for hiding this comment

Uh oh!

BeckerFelix commented Mar 3, 2023

Uh oh!

ejguan commented Mar 3, 2023

Uh oh!

BeckerFelix commented Mar 3, 2023

Uh oh!

ejguan commented Mar 3, 2023

Uh oh!

pytorchmergebot commented Mar 3, 2023

Uh oh!

pytorchmergebot commented Mar 3, 2023

Uh oh!

ejguan commented Mar 3, 2023

Uh oh!

pytorchmergebot commented Mar 3, 2023

Merge started

Uh oh!

ejguan Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

ejguan Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

BeckerFelix commented Feb 17, 2023 •

edited

Loading

pytorch-bot bot commented Feb 17, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 17, 2023 •

edited

Loading

NivekT left a comment •

edited

Loading