Skip to content

Moco Inplace Update Params#1693

Closed
eellison wants to merge 1 commit into
pytorch:mainfrom
eellison:moco_param_change
Closed

Moco Inplace Update Params#1693
eellison wants to merge 1 commit into
pytorch:mainfrom
eellison:moco_param_change

Conversation

@eellison

@eellison eellison commented May 25, 2023

Copy link
Copy Markdown
Contributor

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

xuzhao9 commented May 26, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

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

@xuzhao9

xuzhao9 commented May 26, 2023

Copy link
Copy Markdown
Contributor

@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

eellison commented Jun 5, 2023

Copy link
Copy Markdown
Contributor Author

@xuzhao9 facebookresearch/moco#141

@xuzhao9 xuzhao9 left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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