Skip to content

Upgrade torch nightly version to 2.8.0.dev20250408+cpu#30

Merged
jinevening merged 3 commits intoSamsung:mainfrom
dayo09:0409-upgrade
Apr 10, 2025
Merged

Upgrade torch nightly version to 2.8.0.dev20250408+cpu#30
jinevening merged 3 commits intoSamsung:mainfrom
dayo09:0409-upgrade

Conversation

@dayo09
Copy link
Contributor

@dayo09 dayo09 commented Apr 9, 2025

Let's upgrade torch nightly version to the latest.

TICO-DCO-1.0-Signed-off-by: Dayoung Lee dayoung.lee@samsung.com


Related to: #9

strategy:
matrix:
torch-version: ["2.5", "2.6"]
torch-version: ["2.5", "2.6", "2.7", "nightly"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: pytorch 2.7.0 release date is planed to 4/25, due to https://dev-discuss.pytorch.org/t/pytorch-release-2-7-0-call-for-features/2780.

@dayo09 dayo09 force-pushed the 0409-upgrade branch 2 times, most recently from bd7971e to 8a05322 Compare April 9, 2025 08:26
@dayo09 dayo09 marked this pull request as ready for review April 9, 2025 08:26
mhs4670go
mhs4670go previously approved these changes Apr 9, 2025
Copy link
Contributor

@mhs4670go mhs4670go left a comment

Choose a reason for hiding this comment

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

LGTM

@dayo09 dayo09 requested a review from a team April 10, 2025 01:09
@jinevening
Copy link
Contributor

It seems that this PR includes some chanages about ResizeNearestNeighbor (not sure this is the only change), but they are hidden behind the commit message of "Upgrade torch nightly version".

I suggest to make an issue whenever we upgrade torch version, and list up related issues there. That will be helpful to track our decisions for supporting new torch versions.

@dayo09 Could you make an issue for this update?

@dayo09
Copy link
Contributor Author

dayo09 commented Apr 10, 2025

@jinevening Sorry that I forgot to link the issue #9.


input: torch.fx.Node
dim: int
dim: int = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some operators have void dim field in torch 2.8.dev

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it occur only in SliceArgs? If not, how about to split the PR to set the default value for dim arg?

SegmentIndexSelectConst(),
LegalizeCausalMaskValue(enabled=config.get("legalize_causal_mask_value")),
ConvertIndexToResizeNearestNeighbor(),
LowerToResizeNearestNeighbor(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that Upsample is added to the Lowering to Resize Nearest Neighbor target list, I intergrate them and rename the pass.

# This pass should be run before 'RestoreLinear' and after 'decompose_quantize_op'.
# TODO run pass regardless of the orders.
with SuppressWarning(UserWarning, ".*quantize_per_tensor"):
with SuppressWarning(UserWarning, ".*quantize_per_tensor"), SuppressWarning(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To suppress the warning from 2.8. It seems oneDNN kinda thirdparty feature is added. It's very unnecessary warning for us.

def __init__(self):
super().__init__()

def convert_index_to_resize_nearest_neighbor(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is almost the same to the original code.


return resize_nearest_neighbor

def convert_upsample_nearest2d_to_resize_nearest_neighbor(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is newly added.

Comment on lines +43 to +47
@tag.skip_if(
not HAS_TORCH_OVER_28_DEV,
reason="The case isn't supported yet. It will be supported from torch 2.8.0.dev",
)
class InterpolateOnePointFive(torch.nn.Module):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Q. Why it's not supported in lower version?

A.

  • CASE(torch < 2.8.0.dev)
    • it's converted to index. -> It's interpreted to "Multiple indices" case. -> it's not supported in tico yet.
  • CASE(torch >= 2.8.0.dev)
    • it's converted to upsample nearest 2d. -> it converts directly to RNN(ResizeNearsestNeighbor) in tico. -> Can support 1.5 times.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can I ask what is "Multiple indices" case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jinevening IndexTensorVisitor don't support multiple indices.

        # TODO Support multiple indices
        if len(indices) - indices.count(None) > 1:  # type: ignore[arg-type]
            raise NotYetSupportedError(
                "Multiple indices is not supported yet in aten.index.Tensor"
            )

This is current implementaion. I left it as TODO.
I didn't look into what values were. 🤔

@dayo09 dayo09 requested a review from jinevening April 10, 2025 01:39
from test.utils import tag


class InterpolateDouble(torch.nn.Module):
Copy link
Contributor

Choose a reason for hiding this comment

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

(not in this PR) BTW, it seems that this file can be moved to op, not net.

@jinevening
Copy link
Contributor

Ah, I missed #9 (added a back link to the main comment). I left some comments. PTAL

@dayo09
Copy link
Contributor Author

dayo09 commented Apr 10, 2025

#37 must be merged to resolve the lint error.

class ResizeNearestNeighborArgs:
"""
# Only consider `torch.nn.functional.interpolate(x, scale_factor=2.0, mode='nearest')` case.
# Maps from `torch.nn.functional.interpolate(x, scale_factor=scale_factor, mode='nearest')` case.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Maps from `torch.nn.functional.interpolate(x, scale_factor=scale_factor, mode='nearest')` case.
# Mapped from `torch.nn.functional.interpolate(x, scale_factor=scale_factor, mode='nearest')` case.

(nit)
May be..?

dayo09 and others added 3 commits April 10, 2025 13:08
Let's upgrade torch nightly version to the latest.

TICO-DCO-1.0-Signed-off-by: Dayoung Lee <dayoung.lee@samsung.com>
Co-authored-by: seongwoo chae <mhs4670go@naver.com>
Comment on lines +64 to +65
[BEFORE PASS]
input - aten.index - output
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[BEFORE PASS]
input - aten.index - output
[BEFORE PASS]
case 1.
input - aten.index - output
case 2.
input - aten.upsample_nearest2d.vec - output

It would be nice to add a case for aten.upsample_nearest2d.vec :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me apply in the next PR :-D

Copy link
Contributor

@miusic miusic left a comment

Choose a reason for hiding this comment

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

LGTM with some comments 👍

@dayo09 dayo09 requested review from jinevening and mhs4670go April 10, 2025 04:44
model: torch.nn.Module, example_inputs: tuple, pt2_model_path: str
):
# Create .pt2 model
with torch.no_grad(), SuppressWarning(UserWarning, ".*quantize_per_tensor"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related with this PR but seems that quantize_per_tensor warning has been removed.

Copy link
Contributor

@mhs4670go mhs4670go left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jinevening jinevening left a comment

Choose a reason for hiding this comment

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

LGTM

@jinevening jinevening merged commit f0aeefc into Samsung:main Apr 10, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants