Skip to content

feat(relax/frontend/torch): Add basic range constraint support #17898

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

demoncoder-crypto
Copy link

Addresses #17818

@mshr-h
Copy link
Contributor

mshr-h commented Apr 27, 2025

@demoncoder-crypto
Copy link
Author

done @mshr-h Please check if my implementation is up to expectation

@demoncoder-crypto
Copy link
Author

Understood @mshr-h I will fix it.

@demoncoder-crypto
Copy link
Author

@mshr-h I have made the changes requested by you

@demoncoder-crypto demoncoder-crypto requested a review from mshr-h May 2, 2025 12:40
Copy link
Contributor

@mshr-h mshr-h 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 the update! Please fix the CI errors so that we can merge it. @demoncoder-crypto

@demoncoder-crypto
Copy link
Author

demoncoder-crypto commented May 3, 2025

Tried to fix the errors do let me know if it works now thanks @mshr-h

Edit - i also tried creating helper methods I don't know how to fix the lint error

@demoncoder-crypto demoncoder-crypto force-pushed the fix/relax-pytorch-constraints-v2 branch from 4145c11 to b66a89b Compare May 3, 2025 12:44
@demoncoder-crypto
Copy link
Author

demoncoder-crypto commented May 3, 2025

I cannot fix the lint/pr-head its expecting less nesting which would require me to make a helper method should I proceed and make it? @mshr-h

@demoncoder-crypto demoncoder-crypto force-pushed the fix/relax-pytorch-constraints-v2 branch 4 times, most recently from 594a7e4 to 30d49d8 Compare May 3, 2025 14:43
@demoncoder-crypto demoncoder-crypto requested a review from mshr-h May 3, 2025 14:58
@demoncoder-crypto demoncoder-crypto force-pushed the fix/relax-pytorch-constraints-v2 branch 3 times, most recently from 064f5d7 to dc4ebca Compare May 3, 2025 19:52
@demoncoder-crypto demoncoder-crypto force-pushed the fix/relax-pytorch-constraints-v2 branch from dc4ebca to 5c7758c Compare May 3, 2025 22:22

return parameters_buffers_constants, user_inputs, relax_range_constraints

# NEW HELPER METHOD
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the unnecessary comment.

return parameters_buffers_constants, user_inputs, relax_range_constraints

# NEW HELPER METHOD
def _add_range_constraint(self, constraints_dict, relax_tir_var, min_val, max_val):
Copy link
Contributor

Choose a reason for hiding this comment

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

@staticmethod would be better since it doesn't access the instance variable or method.

@mshr-h
Copy link
Contributor

mshr-h commented May 4, 2025

Adding helper functions is totally fine as long as it's readable and maintainable. @demoncoder-crypto

@mshr-h
Copy link
Contributor

mshr-h commented May 4, 2025

I think we need multiple testcases so that we can cover all the possible cases:

  1. lower constraints only
  2. upper constraints only
  3. both the lower and upper constraints
  4. the constraints are overridden by another constraints (this is already in the test)

@yongwww
Copy link
Member

yongwww commented May 20, 2025

@demoncoder-crypto pls fix the conflicts and ci

Copy link
Contributor

@mshr-h mshr-h left a comment

Choose a reason for hiding this comment

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

Please fix the CI error and resolve the conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants