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

[Reland] fix missing-prototypes warnings in torch_cpu (Part 4) #101949

Closed
wants to merge 36 commits into from

Conversation

cyyever
Copy link
Collaborator

@cyyever cyyever commented May 21, 2023

This PR relands the changes introduced in PR #100849. The old PR turnd nnc_aten_embedding into a static function, however, it is actually used in torch/csrc/jit/tensorexpr/operators/misc.cpp.

cc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10 @EikanWang @izaitsevfb @albanD

@github-actions github-actions bot added NNC release notes: quantization release notes category labels May 21, 2023
@cyyever cyyever changed the title [Reland] fix missing-prototypes warnings in torch_cpu (Part 6) [Reland] fix missing-prototypes warnings in torch_cpu (Part 4) May 21, 2023
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Thanks!
Do you know why this didn't fail in our CI? Do we just never build with tensorexpr in CI?

@cyyever
Copy link
Collaborator Author

cyyever commented May 22, 2023

@albanD We build with tensorexpr but the test coverage isn't 100%. May be we can add the failed test inside MeTa to the OSS version.

@cyyever
Copy link
Collaborator Author

cyyever commented May 22, 2023

@pytorchmergebot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 22, 2023
@pytorchmergebot
Copy link
Collaborator

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

@albanD
Copy link
Collaborator

albanD commented May 22, 2023

May be we can add the failed test inside MeTa to the OSS version.

@izaitsevfb is that something you can look into on your end?

@izaitsevfb
Copy link
Contributor

izaitsevfb commented May 23, 2023

May be we can add the failed test inside MeTa to the OSS version.

@izaitsevfb is that something you can look into on your end?

@albanD, correct me if I'm wrong, but the linker issue seems to be stemming from the way we build things like android and xplat at Meta. It could be theoretically possible to find all PT symbols that we use and pin them somehow in OSS, but that's quite a project. For example, it seems that now we're having issue with another symbol, nnc_prepacked_linear_clamp_run, see D46093855.

We'll try to brainstorm the solution with @osalpekar.

@cyyever, if this PR gets reverted again, please ping me next time before merge, I'll import it manually to see if it breaks anything.

pytorchmergebot added a commit that referenced this pull request May 23, 2023
#101976)"

This reverts commit 4db2dad.

Reverted #101976 on behalf of https://github.com/osalpekar due to reverting to allow #101949 to be cleanly reverted ([comment](#101976 (comment)))
pytorchmergebot referenced this pull request May 23, 2023
PR #101788 depended on #100849 which was  reverted. Now that #100849 has been relanded,  we can reland #101788 too.

Pull Request resolved: #101976
Approved by: https://github.com/Skylion007
@osalpekar
Copy link
Member

@pytorchbot revert -m "As noted in @izaitsevfb's comment, we are still seeing linker errors, this time due to nnc_prepacked_linear_clamp_run being made a static function." -c ghfirst

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@cyyever your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request May 23, 2023
#101949)"

This reverts commit 4f2c007.

Reverted #101949 on behalf of https://github.com/osalpekar due to As noted in @izaitsevfb's comment, we are still seeing linker errors, this time due to `nnc_prepacked_linear_clamp_run` being made a static function. ([comment](#101949 (comment)))
@cyyever
Copy link
Collaborator Author

cyyever commented May 24, 2023

@izaitsevfb It seems that the changes in external_functions.cpp are the only reason this PR got reverted again. Then a simple solution is to discard them in that file.

@izaitsevfb
Copy link
Contributor

@izaitsevfb It seems that the changes in external_functions.cpp are the only reason this PR got reverted again. Then a simple solution is to discard them in that file.

@cyyever, trying that now, will keep you posted.

@izaitsevfb
Copy link
Contributor

@cyyever, the signals looks good after making nnc_prepacked_linear_clamp_run non-static. I believe it's safe to reland this PR. v5 seems to be fine as well. Thank you for your patience.

@osalpekar, please see D46093855.

@cyyever
Copy link
Collaborator Author

cyyever commented May 28, 2023

@izaitsevfb I have create a new PR #102228 for reland.

@cyyever cyyever deleted the missing_include4_reland branch June 15, 2023 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged module: cpu CPU specific problem (e.g., perf, algorithm) NNC open source release notes: linalg_frontend release notes category release notes: quantization release notes category Reverted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants