Skip to content

Conversation

@justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Jan 8, 2023

PyTorch can supply int32 inputs as SymInt values. This change adds explicit casts to them to fix the Windows CI.

Fixed additional mypy errors

#289 may be needed for python3.10

@codecov
Copy link

codecov bot commented Jan 8, 2023

Codecov Report

Merging #292 (6785915) into main (9c75044) will decrease coverage by 0.53%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main     #292      +/-   ##
==========================================
- Coverage   73.26%   72.73%   -0.54%     
==========================================
  Files          95       95              
  Lines        9171     9176       +5     
==========================================
- Hits         6719     6674      -45     
- Misses       2452     2502      +50     
Impacted Files Coverage Δ
onnxscript/backend/onnx_backend.py 78.39% <50.00%> (-0.98%) ⬇️
onnxscript/utils.py 59.37% <70.00%> (-0.95%) ⬇️
onnxscript/function_libs/torch_aten/ops/core.py 60.81% <100.00%> (+0.05%) ⬆️
onnxscript/function_libs/torch_aten/typing.py 100.00% <100.00%> (ø)
onnxscript/test/models/onnxfns1A.py 68.88% <0.00%> (-26.67%) ⬇️
onnxscript/test/functions/onnxfns1A_test.py 82.05% <0.00%> (-15.39%) ⬇️
onnxscript/test/common/onnx_script_test_case.py 79.80% <0.00%> (-12.50%) ⬇️
onnxscript/test/functions/onnxfns_test.py 95.34% <0.00%> (-2.33%) ⬇️
onnxscript/converter.py 90.55% <0.00%> (-1.49%) ⬇️
onnxscript/tensor.py 86.91% <0.00%> (-0.94%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

# FIXME(#277): Script when attributes can come before inputs
@torch_op("aten::index_select", trace_only=True)
def aten_index_select(self: TTensor, dim: int, index: TInt) -> TTensor:
def aten_index_select(self: TTensor, dim: int, index: IntType) -> TTensor:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we keep TInt here? Looks like they are the same to me.

If we are using Txxx in most of places, I'd like to use the same style.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since mypy requires a TypeVar to appear at least twice in the signature of a generic function, we are forced to use a plain type here.

Copy link
Collaborator Author

@justinchuby justinchuby Jan 9, 2023

Choose a reason for hiding this comment

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

Actually, we can probably get away with TInt here. Let me try again and replace as many with TInt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@fatcat-z fatcat-z left a comment

Choose a reason for hiding this comment

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

Approved, thanks!

@justinchuby justinchuby added module: torchlib Related to the torch/aten function lib in development hold on merging labels Jan 9, 2023
@justinchuby justinchuby merged commit 109bf7f into main Jan 9, 2023
@justinchuby justinchuby deleted the justinchu/windows-ci branch January 9, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: torchlib Related to the torch/aten function lib in development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants