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.

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

Feat: Remove QOP Export #917

Merged
merged 6 commits into from
Mar 27, 2024
Merged

Conversation

costigt-dev
Copy link
Collaborator

@costigt-dev costigt-dev commented Mar 22, 2024

Reason for this PR

QOp export is not in use so can be removed to simplify code base

Changes Made in this PR

Remove QOp export code and references in documentation

Testing Summary

Relying on automated testing, functionality is being removed and none is being added

Risk Highlight

  • This PR includes code from another work (please detail).
  • This PR contains API-breaking changes.
  • This PR depends on work in another PR (please provide links/details).
  • This PR introduces new dependencies (please detail).
  • There are coverage gaps not covered by tests.
  • Documentation updates required in subsequent PR.

ONNX and Torch QOp export will no longer be possible

Checklist

  • Code comments added to any hard-to-understand areas, if applicable.
  • Changes generate no new warnings.
  • Updated any relevant tests, if applicable.
  • No conflicts with destination dev branch.
  • I reviewed my own code changes.
  • Initial CI/CD passing.
  • 1+ reviews given, and any review issues addressed and approved.
  • Post-review full CI/CD passing.

Future Work

@Giuseppe5 Giuseppe5 linked an issue Mar 22, 2024 that may be closed by this pull request
@@ -254,8 +205,6 @@ def _cache_inp_out(cls, module, *args, **kwargs):
module.apply(lambda m: _override_inp_caching_mode(m, enabled=True))
module.apply(lambda m: _override_out_caching_mode(m, enabled=True))
with ExitStack() as stack:
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we don't add anything to the stack, we can remove the context manager and leave the forward pass as part of the function

@@ -280,8 +229,6 @@ def jit_inference_trace(
# force requires_grad to False to let the wrapped model lambda go through tracing
requires_grad_backup_dict = _force_requires_grad_false(module)
with ExitStack() as stack:
for mgr in cls._trace_patches():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as before

@@ -123,8 +123,6 @@ def export_onnx(
module.apply(lambda m: _override_inp_caching_mode(m, enabled=False))
# perform export pass
with ExitStack() as stack:
for mgr in cls._trace_patches():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as before, everything can be flattened one level down

@Giuseppe5 Giuseppe5 self-requested a review March 27, 2024 08:58
@Giuseppe5 Giuseppe5 merged commit 3364a92 into Xilinx:dev Mar 27, 2024
341 of 347 checks passed
Giuseppe5 pushed a commit to Giuseppe5/brevitas that referenced this pull request Apr 12, 2024
Giuseppe5 pushed a commit to Giuseppe5/brevitas that referenced this pull request Apr 12, 2024
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.

Deprecate QOp Export
2 participants