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

remove unused _pin_memory property in PinMemory class #1413

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

FightingZhen
Copy link
Contributor

@FightingZhen FightingZhen commented Dec 24, 2024

Please read through our contribution guide prior to
creating your pull request.

  • If you are adding a new node, ensure you read that section in the contribution guide, as it includes requirements for
    functionality and testing.

Fixes #{issue number}

  • Not related

Changes

  • Remove unused _pin_memory property in PinMemory class

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 24, 2024
@divyanshk
Copy link
Contributor

Thanks @FightingZhen for the PR! Can you elaborate more on your use-case for this?

@andrewkho
Copy link
Contributor

I assume it's to get parity with this: https://github.com/pytorch/pytorch/blob/main/torch/utils/data/_utils/pin_memory.py#L31 .

side note @divyanshk we need to do a sync for stateful_dataloader, updating our tests and code to match pytorch core's dataloader v1

@andrewkho
Copy link
Contributor

@FightingZhen any ideas on how to test this? Looks like this PR in pytorch/pytorch landed without tests: pytorch/pytorch@3460b2b

Copy link

pytorch-bot bot commented Dec 27, 2024

🔗 Helpful Links

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

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

❌ 26 New Failures

As of commit f94164b with merge base 62092dd (image):

NEW FAILURES - The following jobs have failed:

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

@FightingZhen FightingZhen changed the title Add privateuse1 situation support for _pin_memory property in PinMemory class remove unused _pin_memory property in PinMemory class Dec 28, 2024
@facebook-github-bot
Copy link
Contributor

@andrewkho has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@andrewkho
Copy link
Contributor

Starting an internal CI run to run GPU tests

@andrewkho
Copy link
Contributor

Wonder if there was a change to CI runners that broke determinism, the broken tests seem to be related to controlled randomness. Either way, I ran the tests after deleting and everything seems to pass so we should be good

Copy link
Contributor

@andrewkho andrewkho left a comment

Choose a reason for hiding this comment

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

LGTM

@FightingZhen
Copy link
Contributor Author

@andrewkho The CI looks finished, I'm not sure whether the above failed jobs is related with my modification or not, can this PR be merged? :)

@andrewkho
Copy link
Contributor

@FightingZhen yup please go ahead and merge, thank you for your contribution!

@FightingZhen
Copy link
Contributor Author

@divyanshk can you help me merge this pr? thanks :)

@andrewkho andrewkho merged commit 3f866a8 into pytorch:main Dec 30, 2024
15 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants