fix: add support for enabling onnx subfunction ci#42
Conversation
Signed-off-by: qraniumcitest <rmakar@qti.qualcomm.com>
Signed-off-by: vbaddi <quic_vbaddi@quicinc.com>
|
Jenkins-Ready |
|
From GHES (comment) by @qraniumtest Jenkins-Ready Source: https://github.qualcomm.com/qranium/efficient-transformers/pull/40#issuecomment-993845 |
|
From GHES (comment) by @qgeniecodeassistant[bot] Code AssistantReviewed Commits: cb7da87, 6b2a01b, bd0d9ef
Updated the correct code with updated syntax, removed device_group Signed-off-by: Sharvari Medhe smedhe@qti.qualcomm.com
Signed-off-by: qraniumcitest rmakar@qti.qualcomm.com
Signed-off-by: vbaddi quic_vbaddi@quicinc.com PR OverviewThis PR introduces significant enhancements to the ONNX export functionality, including support for exporting decoder layers as ONNX functions, custom operation transforms, ONNX graph optimization via onnx-slim, and torch patches for compatibility. It also adds a new GitHub Actions workflow for mirroring fork PRs to GHES. Files Changed Summary
Critical Issues
[Security - Medium] Hardcoded credentials and sensitive information in GitHub workflowThe GitHub Actions workflow file contains hardcoded repository names, organization names, and base URLs that could be security-sensitive. While these may be intentional for the specific use case, it's better practice to use repository variables or secrets for such configuration. Fixed Code Snippet: env:
GHES_BASE_URL: ${{ vars.GHES_BASE_URL }}
GHES_OWNER: ${{ vars.GHES_OWNER }}
GHES_REPO: ${{ vars.GHES_REPO }}
FORK_OWNER: ${{ vars.FORK_OWNER }}
FORK_REPO: ${{ vars.FORK_REPO }}[Functionality - Medium] Debug print statement left in production codeA debug print statement is present in the production code at line 441 of cache_utils.py. This appears to be leftover debugging code that should be removed before merging to production. Fixed Code Snippet: invalid_idx_value = _get_invalid_idx_value()
ctx_indices = torch.where(invalid_mask, invalid_idx_value, ctx_indices)[Maintainability - Medium] Intentional empty dictionary override may cause issuesIn patches.py at line 40, the code intentionally overrides Consider investigating if there's a better way to handle the type mismatch that doesn't discard the attributes entirely, or add comprehensive testing to ensure this doesn't break functionality for models that rely on these attributes. Fixed Code Snippet: # FIX: Convert to compatible type to avoid mismatch with _jit_pass_onnx_track_scope_attributes
# Observed in transformers v4.55 and above
if onnx_attrs and not isinstance(onnx_attrs, dict):
onnx_attrs = {}
_C._jit_pass_onnx_track_scope_attributes(graph, onnx_attrs)[Error Handling - Low] Missing error handling in ONNX transformThe OnnxSlimTransform.apply() method at line 132 calls Fixed Code Snippet: if onnx_slim_transform:
transformed = True
try:
slimmed_model = onnxslim.slim(model)
onnx.save(slimmed_model, temp_onnx_path)
except Exception as e:
logger.warning(f"ONNX slim optimization failed: {e}. Continuing with original model.")
return model, False
return slimmed_model, transformed[Performance - Low] Redundant environment variable check in _get_invalid_idx_valueThe function Fixed Code Snippet: # At module level
_USE_ONNX_FUNCTIONS = os.environ.get("QEFF_USE_ONNX_FUNCTIONS", "false").lower() == "true"
def _get_invalid_idx_value():
"""
Get the appropriate invalid index value for CtxGather operations.
"""
if torch.onnx.is_in_onnx_export():
if _USE_ONNX_FUNCTIONS:
return 0
else:
return torch.iinfo(torch.int32).max
else:
return 0** Version 1.3.6** Help us improve! How useful was this code feedback? Not very useful 1️⃣ 2️⃣ 3️⃣ 4️⃣ 5️⃣ Very useful How much time did it save you (in hours)? 0 | <1 | 1-3 | >4 Let us know your detailed feedback Source: https://github.qualcomm.com/qranium/efficient-transformers/pull/43#issuecomment-1009227 |
2 participants
Jenkins-Ready