Conversation
Signed-off-by: qraniumcitest <rmakar@qti.qualcomm.com>
Signed-off-by: vbaddi <quic_vbaddi@quicinc.com>
Signed-off-by: vbaddi <quic_vbaddi@quicinc.com>
|
From GHES (comment) by @RBagla Code AssistantReviewed Commits: cb7da87, 6b2a01b, bd0d9ef, 7c53cdd
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
Signed-off-by: vbaddi quic_vbaddi@quicinc.com Pull Request OverviewThis PR introduces significant enhancements to the ONNX export pipeline, including support for ONNX functions, custom operations, and improved model transformations. The changes span across multiple core modules including export logic, cache utilities, model transformations, and configuration constants. Files Changed Summary
Critical Issues Identified
Key Accomplishments
[Security] Hardcoded Token Exposure in Debug Logging - Medium SeverityThe GitHub Actions workflow contains debug logging statements that print the length of the Issue Details:
Fixed Code Snippet: RESP="$(curl -sS -H "Authorization: token ${GHES_PAT}" \
-H "Accept: application/vnd.github+json" \
"${API}/pulls?state=open&head=${GHES_OWNER}:${BRANCH}" \
-w "\n%{http_code}")"
HTTP_CODE="$(printf '%s\n' "$RESP" | tail -n1)"
JSON="$(printf '%s\n' "$RESP" | sed '$d')"
echo "HTTP_CODE=${HTTP_CODE}"[Functionality] Missing Error Handling in Custom Operation Registration - Medium SeverityThe Issue Details:
Fixed Code Snippet: @classmethod
def register_custom_op(cls, op_name: str, func_class: Any, onnxscript_func: Any):
"""Register a custom operation."""
if not op_name or not isinstance(op_name, str):
raise ValueError(f"Invalid op_name: {op_name}. Must be a non-empty string.")
if op_name in cls._custom_ops:
logger.warning(f"Custom operation '{op_name}' is already registered. Overwriting.")
if not callable(onnxscript_func):
raise TypeError(f"onnxscript_func for '{op_name}' must be callable")
cls._custom_ops[op_name] = (func_class, onnxscript_func)
logger.debug(f"Registered custom operation: {op_name}")[Performance] Debug Print Statement in Production Code - Low SeverityThe Issue Details:
Fixed Code Snippet: invalid_mask = ctx_indices > gather_limit
invalid_idx_value = _get_invalid_idx_value()
ctx_indices = torch.where(invalid_mask, invalid_idx_value, ctx_indices)[Maintainability] Incomplete Environment Variable Validation - Low SeverityThe Issue Details:
Fixed Code Snippet: def _get_invalid_idx_value():
"""
Get the appropriate invalid index value for CtxGather operations.
For ONNX export with functions, we use 0 to avoid INT32_MAX constants
that cause issues when functions are inlined at runtime.
Returns:
int: Invalid index value (0 for ONNX functions, INT32_MAX otherwise)
"""
if torch.onnx.is_in_onnx_export():
# Check if ONNX functions are being used
use_onnx_functions_str = os.environ.get("QEFF_USE_ONNX_FUNCTIONS", "false").lower()
use_onnx_functions = use_onnx_functions_str in ("true", "1", "yes")
if use_onnx_functions_str not in ("true", "false", "1", "0", "yes", "no"):
import logging
logger = logging.getLogger(__name__)
logger.warning(f"Invalid value for QEFF_USE_ONNX_FUNCTIONS: '{use_onnx_functions_str}'. Using default (false).")
if use_onnx_functions:
# For ONNX functions: use 0 to avoid function inlining issues
return 0
else:
# For regular ONNX export: use INT32_MAX as before
return torch.iinfo(torch.int32).max
else:
# For runtime: use 0
return 0** Version 1.0** 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/42#issuecomment-1007487 |
|
From GHES (comment) by @RBagla Code AssistantReviewed Commits: cb7da87, 6b2a01b, bd0d9ef, 7c53cdd
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
Signed-off-by: vbaddi quic_vbaddi@quicinc.com Pull Request OverviewThis PR introduces significant enhancements to the ONNX export pipeline, including support for ONNX functions, custom operations, and improved model transformations. The changes span across multiple core modules including export logic, cache utilities, model transformations, and configuration constants. Files Changed Summary
Critical Issues Identified
[Security - Medium] Potential Token Exposure in GitHub Workflow Debug LoggingThe GitHub Actions workflow contains debug logging statements that print the length of the GHES_PAT token and make test API calls. While the token itself isn't directly printed, these debug statements could leak information about the token in workflow logs and should be removed before production use. Lines 202-205 contain debugging code that:
Fixed Code Snippet: HTTP_CODE="$(printf '%s\n' "$RESP" | tail -n1)"
JSON="$(printf '%s\n' "$RESP" | sed '$d')"
echo "HTTP_CODE=${HTTP_CODE}"
if [ "${HTTP_CODE}" != "200" ]; then[Functionality - Medium] Missing Error Handling in ONNX Transform OperationsThe Additionally, the Fixed Code Snippet: transformed = False
onnx_slim_transform = True # kwargs.get("enable_onnx_slim_transform", False)
temp_onnx_path = kwargs.get("temp_onnx_path", None)
if not temp_onnx_path:
err_str = "temp_onnx_path is required for onnx-slim transform."
raise RuntimeError(err_str)
if onnx_slim_transform:
try:
transformed = True
slimmed_model = onnxslim.slim(model)
onnx.save(slimmed_model, temp_onnx_path)
return slimmed_model, transformed
except Exception as e:
logger.warning(f"ONNX slim transformation failed: {e}. Returning original model.")
return model, False
return model, transformed[Performance - Low] Debug Print Statement in Production CodeLine 441 in
Fixed Code Snippet: invalid_mask = ctx_indices > gather_limit
invalid_idx_value = _get_invalid_idx_value()
ctx_indices = torch.where(invalid_mask, invalid_idx_value, ctx_indices)[Maintainability - Medium] Incomplete Monkey Patch Implementation May Not Handle All Edge CasesThe monkey patch in
This could lead to unexpected behavior with different versions of transformers or in edge cases where module attributes are actually needed. Fixed Code Snippet: graph = tracing_state.graph()
onnx_attrs = {}
if hasattr(module, attr_name):
onnx_attrs = getattr(module, attr_name)
delattr(module, attr_name)
# FIX: use empty dict to avoid type mismatch with _jit_pass_onnx_track_scope_attributes
# Observed in transformers v4.55 and above
# Only apply fix if onnx_attrs contains non-serializable types
try:
_C._jit_pass_onnx_track_scope_attributes(graph, onnx_attrs)
except (TypeError, RuntimeError):
# Fallback to empty dict if type mismatch occurs
_C._jit_pass_onnx_track_scope_attributes(graph, {})** Version 1.0** 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/42#issuecomment-1007532 |
|
From GHES (comment) by @qgeniecodeassistant[bot] Code AssistantReviewed Commits: cb7da87, 6b2a01b, bd0d9ef, 7c53cdd
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
Signed-off-by: vbaddi quic_vbaddi@quicinc.com Pull Request OverviewThis PR introduces significant enhancements to the ONNX export pipeline, including support for ONNX functions, custom operations, and improved model transformations. The changes span across multiple core modules including export logic, cache utilities, model transformations, and configuration constants. Files Changed Summary
Critical Issues Identified
[Security - Medium] Potential Token Exposure in GitHub Workflow Debug LoggingThe GitHub Actions workflow contains debug logging statements that print the length of the GHES_PAT token and make test API calls. While the token itself isn't directly printed, these debug statements could leak information about the token in workflow logs and should be removed before production use. Lines 202-205 contain debugging code that:
Fixed Code Snippet: HTTP_CODE="$(printf '%s\n' "$RESP" | tail -n1)"
JSON="$(printf '%s\n' "$RESP" | sed '$d')"
echo "HTTP_CODE=${HTTP_CODE}"
if [ "${HTTP_CODE}" != "200" ]; then[Functionality - Medium] Missing Error Handling in ONNX Transform OperationsThe Additionally, the Fixed Code Snippet: transformed = False
onnx_slim_transform = True # kwargs.get("enable_onnx_slim_transform", False)
temp_onnx_path = kwargs.get("temp_onnx_path", None)
if not temp_onnx_path:
err_str = "temp_onnx_path is required for onnx-slim transform."
raise RuntimeError(err_str)
if onnx_slim_transform:
try:
transformed = True
slimmed_model = onnxslim.slim(model)
onnx.save(slimmed_model, temp_onnx_path)
return slimmed_model, transformed
except Exception as e:
logger.warning(f"ONNX slim transformation failed: {e}. Returning original model.")
return model, False
return model, transformed[Performance - Low] Debug Print Statement in Production CodeLine 441 in
Fixed Code Snippet: invalid_mask = ctx_indices > gather_limit
invalid_idx_value = _get_invalid_idx_value()
ctx_indices = torch.where(invalid_mask, invalid_idx_value, ctx_indices)[Maintainability - Medium] Incomplete Monkey Patch Implementation May Not Handle All Edge CasesThe monkey patch in
This could lead to unexpected behavior with different versions of transformers or in edge cases where module attributes are actually needed. Fixed Code Snippet: graph = tracing_state.graph()
onnx_attrs = {}
if hasattr(module, attr_name):
onnx_attrs = getattr(module, attr_name)
delattr(module, attr_name)
# FIX: use empty dict to avoid type mismatch with _jit_pass_onnx_track_scope_attributes
# Observed in transformers v4.55 and above
# Only apply fix if onnx_attrs contains non-serializable types
try:
_C._jit_pass_onnx_track_scope_attributes(graph, onnx_attrs)
except (TypeError, RuntimeError):
# Fallback to empty dict if type mismatch occurs
_C._jit_pass_onnx_track_scope_attributes(graph, {})** 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/47#issuecomment-1010632 |
2 participants
No description provided.