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.

Sign up for GitHub

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

Jump to bottom

Nvfuser python API import fix #94036

Closed
wants to merge 20 commits into from
Closed

Nvfuser python API import fix #94036

wants to merge 20 commits into from

Conversation

jjsjann123
Copy link
Collaborator

  1. Having nvfuser python API import working with both devel and upstream;
  2. Add environment variable to allow custom nvfuser code base to be built with upstream pytorch core.

@pytorch-bot
Copy link

pytorch-bot bot commented Feb 3, 2023 •
edited

🔗 Helpful Links

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

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

✅ No Failures

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

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

@jjsjann123
Copy link
Collaborator Author

This would help our internal container nightly build to merge the nvfuser & pytorch container.

@jjsjann123 jjsjann123 mentioned this pull request Feb 3, 2023
Closed
@davidberard98
Copy link
Contributor

this will need a lint fix - maybe there's a way to skip that check for these import locations?

I'll give @malfet a chance to comment on the other change

@jjsjann123
Copy link
Collaborator Author

Ah, the python import is done lazily in pytorch and that seems to be ok... Let me patch these.

@bdhirsh bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Feb 6, 2023
@jjsjann123 jjsjann123 added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 6, 2023
crcrpar
crcrpar reviewed Feb 6, 2023
View reviewed changes
torch/_prims/nvfuser_executor.py Outdated
@@ -20,11 +20,12 @@

if torch.cuda.is_available():
import nvfuser
from packaging.version import Version
from packaging.version import Version # type: ignore[import]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this repo's way of importing packaging module. at glance,

pytorch/test/onnx/test_verification.py

Line 14 in d2b82fe

from packaging import version
is the only one which directly imports the module w/o guard. The others seem to import it from pkg_resources or #71902

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think upstream relies on pkg_package in absence of packaging, tho I don't know what the difference is. I think I can use the lazy import from torch_version.py to handle this instead.
I'm not sure about the dependency between that with prim. We'll see if it breaks anything 🤞

Will ping you once I have it updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

switched to torch.torch_version.Version instead. Local testing seems to be working fine.

@jjsjann123
Copy link
Collaborator Author

@davidberard98 Do we still have concern on this PR?

malfet
malfet requested changes Feb 8, 2023
View reviewed changes
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Please use more pythonic paradigms(rather then version checks) to check whether old or new nvfuser is around

test/test_prims.py Outdated Show resolved Hide resolved
torch/_prims/nvfuser_prims.py Outdated Show resolved Hide resolved
torch/_prims/nvfuser_prims.py Outdated Show resolved Hide resolved
torch/_prims/nvfuser_executor.py Outdated Show resolved Hide resolved
torch/_prims/nvfuser_executor.py Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@jjsjann123 jjsjann123 requested review from malfet and crcrpar and removed request for crcrpar February 8, 2023 21:16
@jjsjann123
Copy link
Collaborator Author

No idea what's wrong the the linter failing. Looks like infra issue. I'll restart it later.

@jjsjann123
Copy link
Collaborator Author

Patched a linter error that was missed. CI looks good from previous run otherwise.

@jjsjann123
Copy link
Collaborator Author

CI looks green~~ 🥳

pinging @malfet to approve changes per request

@jjsjann123
Copy link
Collaborator Author

Bumping for update. cc'ing @malfet

davidberard98
davidberard98 approved these changes Feb 14, 2023
View reviewed changes
Copy link
Contributor

@davidberard98 davidberard98 left a comment

Choose a reason for hiding this comment

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

looks like @malfet's requested changes have been resolved

@jjsjann123
Copy link
Collaborator Author

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

Successfully rebased nvfuser_patch_python_import onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout nvfuser_patch_python_import && git pull --rebase)

@jjsjann123
Copy link
Collaborator Author

@pytorchbot merge

@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

@jjsjann123 jjsjann123 deleted the nvfuser_patch_python_import branch February 16, 2023 21:48
@msaroufim msaroufim mentioned this pull request Mar 3, 2023
Closed
pruthvistony added a commit to ROCm/pytorch that referenced this pull request May 2, 2023
@pruthvistony
Revert "Nvfuser python API import fix (pytorch#94036)"
af6976a 
This reverts commit 21eb7f7.
jhavukainen pushed a commit to kulinseth/pytorch that referenced this pull request Mar 15, 2024
@jjsjann123 @jhavukainen
Nvfuser python API import fix (pytorch#94036)
e2da4b2 
1. Having nvfuser python API import working with both devel and upstream;
2. Add environment variable to allow custom nvfuser code base to be built with upstream pytorch core.
Pull Request resolved: pytorch#94036
Approved by: https://github.com/malfet, https://github.com/davidberard98
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malfet malfet malfet approved these changes

@davidberard98 davidberard98 davidberard98 approved these changes

@crcrpar crcrpar Awaiting requested review from crcrpar

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jjsjann123 @davidberard98 @pytorchmergebot @malfet @crcrpar @bdhirsh @pytorchbot