[Bug Fix]: qpc sdk config dump issue fixing #379
Conversation
Signed-off-by: Abukhoyer Shaik <quic_abukhoye@quicinc.com>
Signed-off-by: Abukhoyer Shaik <quic_abukhoye@quicinc.com>
Signed-off-by: Abukhoyer Shaik <quic_abukhoye@quicinc.com>
Signed-off-by: Abukhoyer Shaik <quic_abukhoye@quicinc.com>
Signed-off-by: Abukhoyer Shaik <quic_abukhoye@quicinc.com>
| ) | ||
| qnn_sdk_yaml_path = os.path.join(qnn_sdk_path, QnnConstants.QNN_SDK_YAML) | ||
| with open(qnn_sdk_yaml_path, "r") as file: | ||
| qnn_sdk_details = yaml.safe_load(file) |
There was a problem hiding this comment.
qnn_sdk_details is not declared earlier, what if qnn_sdk_yaml_path is None, then this variable will not be initialized and will give error at line 544. Please declare qnn_sdk_details = None before file open.
| else None | ||
| ) | ||
| qaic_version = None | ||
| if sdk_xml_file is not None: |
There was a problem hiding this comment.
What if sdk_xml_file is None?
There was a problem hiding this comment.
Then aic_version would be None.
| @@ -492,7 +492,7 @@ def create_and_dump_qconfigs( | |||
| many other compilation options. | |||
| """ | |||
| qnn_config = compiler_options["qnn_config"] if "qnn_config" in compiler_options else None | |||
| enable_qnn = True if "qnn_config" in compiler_options else None | |||
| enable_qnn = True if "qnn_config" in compiler_options else False | |||
There was a problem hiding this comment.
are we supposed to check 'qnn_config' or 'enable_qnn' compiler_options here? we set enable_qnn flag for switching between qnn and qaic workflow. qnn_config is an optional parameter.
There was a problem hiding this comment.
But this qconfig_dump function is being called from QEFFBaseModel's _compile method in which we have qnn_config and enable_qnn params. Here, we need for dumping the specific configs whether it is aic or qnn.
| @@ -492,7 +492,7 @@ def create_and_dump_qconfigs( | |||
| many other compilation options. | |||
| """ | |||
| qnn_config = compiler_options["qnn_config"] if "qnn_config" in compiler_options else None | |||
There was a problem hiding this comment.
you can replace it as compiler_options.get("qnn_config", None)
| f"QNN_SDK_PATH {qnn_sdk_path} is not set. Please set {QnnConstants.QNN_SDK_PATH_ENV_VAR_NAME}" | ||
| ) | ||
| qnn_sdk_yaml_path = os.path.join(qnn_sdk_path, QnnConstants.QNN_SDK_YAML) | ||
| with open(qnn_sdk_yaml_path, "r") as file: |
There was a problem hiding this comment.
better to create a load yaml method. Check if the file is available, add proper exception handling and load the yaml file. if there is an uncaught exception in this method it will impact the complete run.
| else: | ||
| # Extract QAIC SDK Apps or Platform Version from SDK XML file | ||
| sdk_xml_file = ( |
There was a problem hiding this comment.
- create a method for fetching the apps and platform sdk.
- Its better to capture platform sdk as well if its installed. it will be useful while debugging
| root = tree.getroot() | ||
| qaic_version = root.find(".//base_version").text | ||
|
|
||
| aic_compiler_config = { |
There was a problem hiding this comment.
These details will not be captured while qnn is enabled. Which is not correct. its better to keep sdk check and others outside as we had done it earlier. if SDK are installed, we will capture that
This PR addresses an issue causing the qconfig dump to fail when QAIC is not installed but QNN is present. With this update, the qconfig will be successfully dumped in both QAIC and QNN environments.