Skip to content

Conversation

xadupre
Copy link
Member

@xadupre xadupre commented Oct 6, 2025

No description provided.

Copy link

codecov bot commented Oct 6, 2025

Codecov Report

❌ Patch coverage is 79.01235% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.38%. Comparing base (75b3d42) to head (988e9f6).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
onnxscript/function_libs/torch_lib/ops/core.py 79.01% 9 Missing and 8 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2606      +/-   ##
==========================================
- Coverage   70.38%   70.38%   -0.01%     
==========================================
  Files         222      222              
  Lines       26288    26390     +102     
  Branches     2629     2647      +18     
==========================================
+ Hits        18503    18574      +71     
- Misses       6865     6886      +21     
- Partials      920      930      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

<https://github.com/pytorch/pytorch/blob/main/torch/onnx/symbolic_opset11.py#L212>`_.
"""
if len(indices) > 1 and any(
isinstance(indice, torch.onnx._internal.exporter._tensors.SymbolicTensor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

check not isinstance(index, int) instead as we should not reference the private class.

Copy link
Member Author

Choose a reason for hiding this comment

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

What are the possible types here? Only int and SymbolicTensor? I prefer to keep SymbolicTensor because I know exactly which type the function is supposed to handle.

Copy link
Collaborator

@justinchuby justinchuby Oct 7, 2025

Choose a reason for hiding this comment

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

Only int and SymbolicTensor for the exporter, but could be other ir.Value subclasses as well. As the type is an internal type it is not meant for public use, the current usage is not supported and is brittle.

If preferred you may check for ir.Value instead, but really we just assume a type that has shape and dtype fields

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to suggest. I still prefer to keep this one since this is what shows up in the error message.

return op.ReduceProd(op.Shape(x, start=i + 1), keepdims=1)

shape = [1] * (len(x.shape) + 1)
mfixed = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer clear variable names and avoid abbreviations.

Copy link
Member Author

@xadupre xadupre Oct 7, 2025

Choose a reason for hiding this comment

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

The variable are used just after. It is short to make the code shorter and esier to read.

Copy link
Collaborator

@justinchuby justinchuby Oct 7, 2025

Choose a reason for hiding this comment

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

Any abbreviation adds cognitive load and is not preferred. All names should be thoughtfully created to be precise. Creating variables used right after is a plus but I don’t feel warranting the use of ambiguous names.

https://google.github.io/styleguide/pyguide.html#316-naming

Copy link
Member Author

Choose a reason for hiding this comment

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

I let you choose the name.

@xadupre xadupre marked this pull request as ready for review October 7, 2025 11:14
Copy link
Collaborator

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

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

As commented:

  1. avoid referencing internal members
  2. follow existing coding style for choosing names for consistency and readability

@github-project-automation github-project-automation bot moved this from Todo to In Progress in ONNX Script Review Board Oct 10, 2025
"""
if (
len(indices) > 1
and any(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a pointer to the real use-cases that show up? I thought each index in indices is supposed to be a 1D tensor? What are the ways in which SymbolicTensors are created?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is a SymbolicTensor a 1-element tensor version of a SymbolicInt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

SymbolicTensor is just ir.Value with magic methods defined to support python operators.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here the symbolic tensors are created from the symints.

@xadupre xadupre closed this Oct 17, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in ONNX Script Review Board Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants