Skip to content

Conversation

@eellison
Copy link
Contributor

@eellison eellison commented May 25, 2023

The out of place .data manipulation is slower/more memory, and doesnt work with cudagraphs.

Upstream pr link: facebookresearch/moco#141

@eellison eellison requested a review from anijain2305 May 25, 2023 23:51
@eellison eellison changed the title Moco change Moco Inplace Update Params May 25, 2023
@xuzhao9
Copy link
Contributor

xuzhao9 commented May 26, 2023

The original code is at https://github.com/facebookresearch/moco/blob/main/moco/builder.py#L63.
Conventionally, we require changes to upstream model code to also create a PR to the upstream repo (in this case, facebookresearch/moco), and reference the upstream PR link in this PR.

Can you help submit a upstream PR and add link to builder.py?

@eellison
Copy link
Contributor Author

That model is last updated in january, don't know if it's really active.

@xuzhao9
Copy link
Contributor

xuzhao9 commented May 26, 2023

@eellison Sorry can you still create a PR? We would like to use it track the patches we've made towards the upstream repo.

@eellison
Copy link
Contributor Author

eellison commented Jun 5, 2023

@xuzhao9 facebookresearch/moco#141

Copy link
Contributor

@xuzhao9 xuzhao9 left a comment

Choose a reason for hiding this comment

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

LGTM, please update the upstream PR link to the code, thanks!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@eellison merged this pull request in 66050c2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants