-
Notifications
You must be signed in to change notification settings - Fork 74.5k
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.
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
Removed trival warnings #27073
Removed trival warnings #27073
Conversation
ae410c5
to
cd5b1ac
Compare
@jdduke , thanks for pointing this out, i updated the code as per the review comments, kindly check and approve. Regards |
cd5b1ac
to
511074d
Compare
@jdduke , i have updated the code as per comments, kindly check and update. Regards |
89b1d54
to
60c060b
Compare
@jdduke , i found one problem in the function CalculateDeallocationOfInternalTensors and CalculateAllocationOfInternalTensors, this is when the node_index becomes negative(in one of the TC) then dues to the implicit conversion with the unsigned int, negative values were becoming positive and hence the test case was passing, not sure this was intentional or oversight, anyway i changed the code as per your suggestion and added the small error check code to ensure sanity. Regards |
@jdduke , i found one problem in the function CalculateDeallocationOfInternalTensors and CalculateAllocationOfInternalTensors, this is when the node_index becomes negative(in one of the TC) then dues to the implicit conversion with the unsigned int, negative values were becoming positive and hence the test case was passing, not sure this was intentional or oversight, anyway i changed the code as per your suggestion and added the small error check code to ensure sanity. Regards |
Interesting, can we expand the test to catch this? Can you tell if this would happen for a legit model/interpreter use-case? |
60c060b
to
fdac508
Compare
@jdduke this happens when the graph is empty , the TC which is triggering this condition is (ArenaPlannerTest, EmptyGraph) , which ultimately calls CalculateAllocations which subtracts one from the active node hence causing the problem. Regards |
Can you add a (unit) test for this case? |
@jdduke , thanks for the review i have added one case to cover this scenario, kindly check and approve. Regards |
8316a55
to
f20d1f7
Compare
@jdduke , sorry for the miss,i have updated the code with the comments, explaining why this TC is added and what problem previously was seen. Hope this is ok. Regards |
@jdduke , thanks for approving the PR, can you please help to get this merged. Regards |
@rthadur is working to land this. |
@rthadur , can you please help to get this merged. Regards |
Its failing because of internal error . Here is the error: |
Removed the warning from the file.
Added a Bug Fix, TC and removed warnings from the file.
aafbd3f
to
fa52c1c
Compare
Changes are merged internally , waiting for auto merge to happen. |
PiperOrigin-RevId: 261385961
Removed some warnings from the files