Skip to content

[Deploy] Change numModules type to unsigned #74978

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
Closed
malfet wants to merge 1 commit into from
Closed

[Deploy] Change numModules type to unsigned #74978

malfet wants to merge 1 commit into from

Conversation

@malfet
Copy link
Contributor

@malfet malfet commented Mar 30, 2022 •
edited
Loading

As it should never be negative, should it?
Also, add torch/csrc/deploy to the list of clang-format checked folders (as they are internally)

Last but not least: clang-tidy correctly identifies totNumModules <= SIZE_MAX / sizeof(struct _frozen) - 1 as unneeded always true check (as totNumModules is int32, while SIZE_MAX is int64 and sizeof(sturct_frozen) is less than 4Gb ;) )

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 30, 2022 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 3082d77 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

❄️ 1 failure tentatively classified as flaky

but reruns have not yet been triggered to confirm:

See GitHub Actions build pull / linux-xenial-py3.7-clang7-asan / test (default, 2, 3, linux.2xlarge) (1/1)

Step: "Test" (full log | diagnosis details | 🔁 rerun) ❄️

2022-03-31T09:40:05.0290690Z unknown file: Failure 
2022-03-31T09:40:04.3609950Z �[0;32m[       OK ] �[mKernel.CatInputTypesPromotion (63 ms)
2022-03-31T09:40:04.3610482Z �[0;32m[ RUN      ] �[mKernel.CatAndInlineWithAConstantDim
2022-03-31T09:40:04.7016455Z �[0;32m[       OK ] �[mKernel.CatAndInlineWithAConstantDim (340 ms)
2022-03-31T09:40:04.7016988Z �[0;32m[ RUN      ] �[mKernel.CatWithEmptyInputs
2022-03-31T09:40:04.9066515Z �[0;32m[       OK ] �[mKernel.CatWithEmptyInputs (204 ms)
2022-03-31T09:40:04.9066895Z �[0;32m[ RUN      ] �[mKernel.CatWoConditionals
2022-03-31T09:40:04.9674328Z �[0;32m[       OK ] �[mKernel.CatWoConditionals (60 ms)
2022-03-31T09:40:04.9674676Z �[0;32m[ RUN      ] �[mKernel.OptimizeConditionals
2022-03-31T09:40:05.0184971Z �[0;32m[       OK ] �[mKernel.OptimizeConditionals (50 ms)
2022-03-31T09:40:05.0185263Z �[0;32m[ RUN      ] �[mKernel.Stack
2022-03-31T09:40:05.0290690Z unknown file: Failure
2022-03-31T09:40:05.0291051Z C++ exception with description "Expected to not find "\n" but found it
2022-03-31T09:40:05.0292431Z       for (int64_t k = 0ll; k < 2ll; k++) {
2022-03-31T09:40:05.0292730Z         for (int64_t l = 0ll; l < 3ll; l++) {
2022-03-31T09:40:05.0293134Z           for (int64_t m = 0ll; m < 6ll; m++) {
2022-03-31T09:40:05.0294364Z             aten_stack[(((108ll * i + 18ll * k) + m) + 36ll * j) + 6ll * l] = k==1ll ? (ty_1[((18ll * j + m) + 54ll * i) + 6ll * l]) : (tx_1[((18ll * j + m) + 54ll * i) + 6ll * l]);
2022-03-31T09:40:05.0294740Z           }
2022-03-31T09:40:05.0294906Z         }
2022-03-31T09:40:05.0295172Z From CHECK-NEXT: aten_stack
2022-03-31T09:40:05.0295360Z " thrown in the test body.
2022-03-31T09:40:05.0295626Z �[0;31m[  FAILED  ] �[mKernel.Stack (10 ms)
This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 30, 2022

@malfet has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@malfet malfet requested review from suo and wconstab March 30, 2022 20:26
@malfet malfet force-pushed the malfet/deploy-numModules-unsigned branch from cff330b to 9962797 Compare March 30, 2022 22:29
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 30, 2022

@malfet has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@malfet malfet force-pushed the malfet/deploy-numModules-unsigned branch from 9962797 to 21b720e Compare March 31, 2022 04:08
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 31, 2022

@malfet has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@malfet malfet force-pushed the malfet/deploy-numModules-unsigned branch from 21b720e to a1ee26c Compare March 31, 2022 06:15
@malfet
[Deploy] Change numModules type to unsigned
3082d77 
As it should never be negative, should it?
@malfet malfet force-pushed the malfet/deploy-numModules-unsigned branch from a1ee26c to 3082d77 Compare March 31, 2022 06:28
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 31, 2022

@malfet has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot pushed a commit that referenced this pull request Mar 31, 2022
@malfet @facebook-github-bot
[Deploy] Change numModules type to unsigned (#74978)
21f6c33 
Summary:
As it should never be negative, should it?
Also, add `torch/csrc/deploy` to the list of clang-format checked folders (as they are internally)

Last but not least: clang-tidy correctly identifies `totNumModules <= SIZE_MAX / sizeof(struct _frozen) - 1` as unneeded always true check (as `totNumModules` is int32, while SIZE_MAX is int64 and `sizeof(sturct_frozen)` is less than 4Gb ;) )

Pull Request resolved: #74978

Reviewed By: suo, tugsbayasgalan

Differential Revision: D35261476

Pulled By: malfet

fbshipit-source-id: 8a3432d2d9e96ded3f08baee14ccb43d2635a67d
@github-actions
Copy link
Contributor

github-actions bot commented Mar 31, 2022

Hey @malfet.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@malfet malfet added release notes: build release notes category topic: bug fixes topic category labels Mar 31, 2022
@malfet malfet deleted the malfet/deploy-numModules-unsigned branch July 17, 2022 18:13
@WBobby WBobby mentioned this pull request Aug 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@suo suo suo approved these changes

@tugsbayasgalan tugsbayasgalan tugsbayasgalan approved these changes

@wconstab wconstab Awaiting requested review from wconstab

Assignees

No one assigned

Labels

cla signed release notes: build release notes category topic: bug fixes topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@malfet @facebook-github-bot @suo @tugsbayasgalan