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

fix: Repair version checking system for Torch #2118

Merged
merged 2 commits into from
Jul 18, 2023
Merged

fix: Repair version checking system for Torch #2118

merged 2 commits into from
Jul 18, 2023

Conversation

gs-olive
Copy link
Collaborator

@gs-olive gs-olive commented Jul 17, 2023 •
edited
Loading

Description

  • Move _C utilities into ts directory
  • Address version parsing issue for NV versions of Torch
  • Add specialized check for NV Torch versions such as 2.0.0.nv23.05

Versions such as 2.0.0.nv23.05 are incompatible with the packaging.version semantic style, and cause invalid version errors upon parsing. Adapting the solution proposed in #2112, we are able to resolve this issue by removing the specialized portion of the version, leaving only the major, minor, and patch values (x.y.z).

Fixes #2112

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • [ x ] My code follows the style guidelines of this project (You can use the linters)
  • [ x ] I have performed a self-review of my own code
  • [ x ] I have commented my code, particularly in hard-to-understand areas and hacks
  • [ x ] I have made corresponding changes to the documentation
  • [ x ] I have added tests to verify my fix or my feature
  • [ x ] New and existing unit tests pass locally with my changes
  • [ x ] I have added the relevant labels to my PR in so that relevant reviewers are notified

@gs-olive gs-olive self-assigned this Jul 17, 2023
@github-actions github-actions bot requested a review from frank-wei July 17, 2023 18:00
narendasan
narendasan reviewed Jul 17, 2023
View reviewed changes
py/torch_tensorrt/__init__.py Outdated
@@ -94,7 +94,11 @@ def _find_lib(name, paths):

from torch_tensorrt import fx

if version.parse(torch.__version__) >= version.parse("2.1.dev"):
if version.parse(
torch.__version__
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make a little macro function in utils that just returns the cleaned torch version

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like

# //py/torch_tensorrt/_utils.py
import torch

def sanitized_torch_version() -> str:
    return torch.__version__ if ".nv" not in torch.__version__ else torch.__version__.split(".nv")[0]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could this function instead go in py/torch_tensorrt/_version.py, to avoid the from torch_tensorrt import _C which occurs in the py/torch_tensorrt/_util.py file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

_version.py gets generated by setup.py so probably not a great choice, but i dont really care where it goes, could go in __init__.py or somewhere else

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, we probably should be moving _C dep code into .ts anyway. Half the functions could probably be reimplemented without _C

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/pytorch/TensorRT/blob/d4c9c06213602c6e8d2ce568154ed287ee67dafd/py/torch_tensorrt/_util.py#L12C5-L12C19 This code just references https://github.com/pytorch/TensorRT/blob/main/core/util/build_info.h

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think lets put it in _util.py with an AI to move the _C code out pre 2.1

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gs-olive add a task to do this in #2119

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This makes sense, I can add the task. I did some minor refactoring of _util.py in this PR since it was a minimal change. Let me know if this change is within the scope of the PR/is favorable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added #2121 and rolled back _util.py refactoring changes initially made in 38a2c4c, since they are out of scope for this PR.

@gs-olive
fix: Repair version checking system for Torch
38a2c4c 
- Move _C utilities into `ts` directory
- Address version parsing issue for NV versions of Torch
- Add specialized check for NV Torch versions such as `2.0.0.nv23.05`
@gs-olive gs-olive mentioned this pull request Jul 18, 2023
Closed
@gs-olive gs-olive requested a review from narendasan July 18, 2023 00:10
narendasan
narendasan approved these changes Jul 18, 2023
View reviewed changes
Copy link
Collaborator

@narendasan narendasan left a comment

Choose a reason for hiding this comment

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

LGTM

@gs-olive gs-olive merged commit e884820 into pytorch:main Jul 18, 2023
@gs-olive gs-olive deleted the version_check_fix branch July 18, 2023 17:24
@janblumenkamp janblumenkamp mentioned this pull request Sep 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@narendasan narendasan narendasan approved these changes

@frank-wei frank-wei Awaiting requested review from frank-wei

Assignees

@gs-olive gs-olive

Labels
cla signed component: api [Python] Issues re: Python API component: fx fx
Projects
None yet
Milestone
No milestone
3 participants
@gs-olive @narendasan @facebook-github-bot