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

Add support for PETR Model(Vovnet based varaints) #1229

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add support for PETR Model(Vovnet based varaints) #1229

wants to merge 1 commit into from

Conversation

kamalrajkannan78
Copy link
Contributor

@kamalrajkannan78 kamalrajkannan78 commented Feb 15, 2025 •
edited
Loading

Summary :

This PR adds support for PETR's vovnet based variants #955

The following modifications have been made to the model code:

  1. In forge/test/models/pytorch/vision/petr/utils/petr_head.py , following in-place operations are replaced with out-of-place operations using torch.cat to avoid pcc drop
  1. In forge/test/models/pytorch/vision/petr/utils/petr_transformer.py, zeros_like op replaced with zeros op on this line to avoid below issue Attribute error in detr #1094
self = Op(zeros), name = 'name_hint'

    def __getattr__(self, name):
        # specially check handle since
        # this is required for PackedFunc calls
        if name == "handle":
            raise AttributeError("handle is not set")
    
        try:
            return _ffi_node_api.NodeGetAttr(self, name)
        except AttributeError:
>           raise AttributeError(f"{type(self)} has no attribute {name}") from None
E           AttributeError: <class 'tvm.ir.op.Op'> has no attribute name_hint
  1. In forge/test/models/pytorch/vision/petr/utils/petr_head.py , this mask creation line leads to below correlation pattern after this operation which ultimately leads to pcc drop. since the mask is derived from shape information rather than actual tensor values, the issue was unable to reproduce using sanity test which leads to AssertionError: There should only be one tt function. To mitigate this, the mask creation was moved outside the model and is now passed as an input to model, resolving below issue.
golden=tensor([[[-2121.4050, -2128.1680, -2132.6357,  ...,  -513.5059,
           -505.7989,  -497.9697],
         [  219.4366,   204.6644,   192.2114,  ..., -1390.2446,
          -1403.7959, -1416.2865],
         [-1758.0610, -1773.0300, -1785.7769,  ...,  -676.3320,
           -669.4155,  -662.0880],
         ...,
         [-2948.6033, -2903.0217, -2855.0762,  ...,  -114.1781,
           -119.5653,  -124.5871],
         [ -190.0512,  -197.2419,  -203.9425,  ...,  -649.5950,
           -655.3975,  -659.3043],
         [ -295.6279,  -293.2321,  -290.6385,  ...,  -463.7845,
           -470.7604,  -475.8008]]])


calculated=tensor([[[-12121.4043, -12128.1680, -12132.6348,  ..., -10513.5059,
          -10505.7988, -10497.9697],
         [ -9780.5635,  -9795.3359,  -9807.7891,  ..., -11390.2441,
          -11403.7959, -11416.2871],
         [-11758.0605, -11773.0293, -11785.7764,  ..., -10676.3320,
          -10669.4160, -10662.0879],
         ...,
         [-12948.6035, -12903.0215, -12855.0762,  ..., -10114.1777,
          -10119.5654, -10124.5869],
         [-10190.0508, -10197.2422, -10203.9424,  ..., -10649.5947,
          -10655.3975, -10659.3047],
         [-10295.6279, -10293.2324, -10290.6387,  ..., -10463.7842,
          -10470.7607, -10475.8008]]], grad_fn=<StackBackward0>)

Note:

  • Unable to brinup resnet based variants due to Issues in bringing up PETR resnet based variants #1230
  • Pretrained weights are not loaded into model.
  • The required versions of mmdet (v2.24.1) and mmdet3d (v0.17.1) cannot be installed in our current PyTorch environment. Upgrading to the latest versions introduces significant code changes, requiring modifications such as updating import paths etc.. on model code. Additionally, mmdet3d requires a GPU for the build setup, making installation impractical. To bypass GPU and installation dependencies, essential scripts from these packages have been isolated. However, both mmdet and mmdet3d contain multiple scripts and directories with the same names. Moving them into the utils directory would create confusion. To maintain clarity, the original directory structure has been preserved, including only the necessary scripts.Meanwhile, required scripts from PETR have been moved to the utils directory since they do not name conflicts.

Logs

@kamalrajkannan78 kamalrajkannan78 force-pushed the kkannan/PETR_vovnet_variants_bringup branch from e4c36c7 to 8685b61 Compare February 15, 2025 12:24
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran518 passed142 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran476 passed125 skipped0 failed
TestResult
No test annotations available

1 similar comment
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran476 passed125 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran518 passed142 skipped0 failed
TestResult
No test annotations available

@kamalrajkannan78 kamalrajkannan78 force-pushed the kkannan/PETR_vovnet_variants_bringup branch from 8685b61 to 16b18b8 Compare February 16, 2025 06:02
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran518 passed142 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran476 passed125 skipped0 failed
TestResult
No test annotations available

1 similar comment
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran476 passed125 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran518 passed142 skipped0 failed
TestResult
No test annotations available

@kamalrajkannan78 kamalrajkannan78 force-pushed the kkannan/PETR_vovnet_variants_bringup branch from 16b18b8 to cc76216 Compare February 16, 2025 08:19
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran476 passed125 skipped0 failed
TestResult
No test annotations available

1 similar comment
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran476 passed125 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran518 passed142 skipped0 failed
TestResult
No test annotations available

1 similar comment
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran518 passed142 skipped0 failed
TestResult
No test annotations available

@kamalrajkannan78 kamalrajkannan78 force-pushed the kkannan/PETR_vovnet_variants_bringup branch 2 times, most recently from 4c75292 to 5cb5f67 Compare February 17, 2025 06:29
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran476 passed125 skipped0 failed
TestResult
No test annotations available

1 similar comment
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran476 passed125 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran518 passed142 skipped0 failed
TestResult
No test annotations available

1 similar comment
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran518 passed142 skipped0 failed
TestResult
No test annotations available

ashokkumarkannan1
ashokkumarkannan1 requested changes Feb 17, 2025
View reviewed changes
Copy link
Contributor

@ashokkumarkannan1 ashokkumarkannan1 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @kamalrajkannan78 . Here are few refactoring comments.

@kamalrajkannan78 kamalrajkannan78 force-pushed the kkannan/PETR_vovnet_variants_bringup branch 2 times, most recently from 72a317d to 83a869f Compare February 17, 2025 14:08
ashokkumarkannan1
ashokkumarkannan1 approved these changes Feb 17, 2025
View reviewed changes
Copy link
Contributor

@ashokkumarkannan1 ashokkumarkannan1 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding the support @kamalrajkannan78

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran520 passed140 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran480 passed121 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran520 passed140 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran480 passed121 skipped0 failed
TestResult
No test annotations available

@kamalrajkannan78 kamalrajkannan78 marked this pull request as ready for review February 17, 2025 16:16
@kamalrajkannan78 kamalrajkannan78 force-pushed the kkannan/PETR_vovnet_variants_bringup branch 2 times, most recently from f07ec56 to 121c959 Compare February 18, 2025 14:04
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran480 passed121 skipped0 failed
TestResult
No test annotations available

1 similar comment
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests601 ran480 passed121 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran520 passed140 skipped0 failed
TestResult
No test annotations available

1 similar comment
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests660 ran520 passed140 skipped0 failed
TestResult
No test annotations available

@kamalrajkannan78
Copy link
Contributor Author

kamalrajkannan78 commented Feb 19, 2025 •
edited
Loading

@ashokkumarkannan1 @vkovinicTT @nvukobratTT
I missed to mention the following change in description earlier

@kamalrajkannan78 kamalrajkannan78 force-pushed the kkannan/PETR_vovnet_variants_bringup branch from 121c959 to 8ac9fff Compare February 19, 2025 20:58
nvukobratTT
nvukobratTT reviewed Feb 24, 2025
View reviewed changes
Copy link
Contributor

@nvukobratTT nvukobratTT left a comment

Choose a reason for hiding this comment

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

As there are ton on new files that are mostly dependeon on third-party GitHub code, let's wait with this one a bit before we set a system for these kind of models.

More precisely, we're in discussion of having separate repository to host this king of models. Therefore, can we mark this PR as draft, until we get new repository set up?

Once that's done, we can push further this model with much less code on FFE side.

What you think @kamalrajkannan78 ?

@kamalrajkannan78 kamalrajkannan78 marked this pull request as draft February 24, 2025 12:07
@kamalrajkannan78
Copy link
Contributor Author

kamalrajkannan78 commented Feb 24, 2025 •
edited
Loading

As there are ton on new files that are mostly dependeon on third-party GitHub code, let's wait with this one a bit before we set a system for these kind of models.

More precisely, we're in discussion of having separate repository to host this king of models. Therefore, can we mark this PR as draft, until we get new repository set up?

Once that's done, we can push further this model with much less code on FFE side.

What you think @kamalrajkannan78 ?

Marked the PR as draft .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvukobratTT nvukobratTT nvukobratTT left review comments

@ashokkumarkannan1 ashokkumarkannan1 ashokkumarkannan1 approved these changes

@vkovinicTT vkovinicTT vkovinicTT approved these changes

@mstojkovicTT mstojkovicTT Awaiting requested review from mstojkovicTT mstojkovicTT is a code owner

@pilkicTT pilkicTT Awaiting requested review from pilkicTT pilkicTT is a code owner

@dgolubovicTT dgolubovicTT Awaiting requested review from dgolubovicTT dgolubovicTT is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kamalrajkannan78 @nvukobratTT @ashokkumarkannan1 @vkovinicTT