Qualcomm AI Engine Direct - alias_copy op

[winskuo-quic]

Summary

Remove alias_copy op.

Test plan

Add UTs to ensure alias_copy is removed.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10319

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 4b587c0 with merge base 4e38f4a ():

NEW FAILURE - The following job has failed:

• pull / test-llava-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 92c37ffcdda88d206e2c3b3eef3e8691e272d1a47ad1f315c6d306e7b8682a1d /exec failed with exit code 139

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[winskuo-quic]

Hi @cccclai, @billmguo,
This PR is the remove alias_copy operation.
Please have a look.
Thanks

[billmguo]

Work for me

[cccclai]

Are we testing alias op should still work, meaning they're not removed?

[winskuo-quic]

Hi @cccclai,
Thanks for reviewing the PR.
Alias op will be removed.
This test is just to show alias can be properly removed and model is still working properly.
I have added some comments under the model.

[cccclai]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

[winskuo-quic]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

This test it to reproduce @billmguo error.
Without adding exir_ops.edge.aten.alias_copy.default to RemoveRedundancy pass, this test will fail during qnn_partitioner where it does not have op_builder for aten.alias_copy.default.

Thanks for the suggestion. That will probably be more straight forward as the unit test is not actually running the alias_op since it is dropped during RemoveRedundancy. Maybe we can add a new class for UT, targeting whether passes are working as expected.

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cccclai]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

[winskuo-quic]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

Thanks for sharing the results. I think this is failing by chances. I tested locally a couple times, and it all passed. I have changed to a simpler OP instead since the purpose here is to verify alias_copy is handled properly.
Would you mind if I push another PR for UT refactor to add a new class for passes related stuff later on? I think we will need to discuss internally first on how to refactor the UT classes.
Thanks

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

[tugsbayasgalan]

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

Hi @cccclai, @tugsbayasgalan,

Yes I am able to reproduce the issue after bumping the torch nightly version to "dev20250422".

I believe the reason is that we are trying to convert the quant weights back to floating point during

executorch/backends/qualcomm/_passes/annotate_quant_attrs.py

Line 144 in d31ef13

set_parameter(param, n.args[0], self.edge_program)

However, after the bumping the torch version, it seems like the FP weights are not properly saved.
We are using the following functions to get and set parameters.

executorch/backends/qualcomm/builders/utils.py

Line 30 in d31ef13

defget_parameter(

executorch/backends/qualcomm/builders/utils.py

Line 47 in d31ef13

defset_parameter(

It would be appreciated if you could share the best practices on handling parameters modification.
Thanks

[tugsbayasgalan]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

[winskuo-quic]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

I think I might know why it is not updating, but I need some time to verify if it is actually the root cause as I am currently blocked by something else.
I think when we are initializing the passes, we passed the exported_program into the constructor.

executorch/backends/qualcomm/_passes/qnn_pass_manager.py

Line 160 in 9e59c19

kwargs["edge_program"] =exported_program

Next, during the lowering process, when decomposition is called, we get a new exported_program with a shallow copy new_state_dict. https://github.com/pytorch/pytorch/blob/ad81eeb7c7c906e0cdd04a5cc8fdb9592281c317/torch/export/exported_program.py#L880
Then, when we are running the passes, we are updating our values to old state_dict in the old exported_program.
This is why we keep on getting incorrect params during QNN op builder.

Please let me know if there's anything that is still unclear.
Thanks

[tugsbayasgalan]

Ahh this makes sense. You should always work with state_dict of ep after running decompositions because the state dict can change after decompositions.

[tugsbayasgalan]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

[winskuo-quic]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

Hi @tugsbayasgalan,
Thanks for the suggestions.
We will work on Qualcomm code to make it compatible with new export behavior.
As you mentioned, it is probably safer to work on state dict of the ep after decomposition.

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

Sorry for the delay on pushing the PR.
We came up with couple solutions and were discussing internally which one is the best to resolve the uplevel error.
We will share the PR later today.

[cccclai]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

[winskuo-quic]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

Just rebased. Thanks for reminder

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[winskuo-quic]

Summary

Remove alias_copy op.

Test plan

Add UTs to ensure alias_copy is removed.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10319

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 4b587c0 with merge base 4e38f4a ():

NEW FAILURE - The following job has failed:

• pull / test-llava-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 92c37ffcdda88d206e2c3b3eef3e8691e272d1a47ad1f315c6d306e7b8682a1d /exec failed with exit code 139

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[winskuo-quic]

Hi @cccclai, @billmguo,
This PR is the remove alias_copy operation.
Please have a look.
Thanks

[billmguo]

Work for me

[cccclai]

Are we testing alias op should still work, meaning they're not removed?

[winskuo-quic]

Hi @cccclai,
Thanks for reviewing the PR.
Alias op will be removed.
This test is just to show alias can be properly removed and model is still working properly.
I have added some comments under the model.

[cccclai]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

[winskuo-quic]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

This test it to reproduce @billmguo error.
Without adding exir_ops.edge.aten.alias_copy.default to RemoveRedundancy pass, this test will fail during qnn_partitioner where it does not have op_builder for aten.alias_copy.default.

Thanks for the suggestion. That will probably be more straight forward as the unit test is not actually running the alias_op since it is dropped during RemoveRedundancy. Maybe we can add a new class for UT, targeting whether passes are working as expected.

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cccclai]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

[winskuo-quic]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

Thanks for sharing the results. I think this is failing by chances. I tested locally a couple times, and it all passed. I have changed to a simpler OP instead since the purpose here is to verify alias_copy is handled properly.
Would you mind if I push another PR for UT refactor to add a new class for passes related stuff later on? I think we will need to discuss internally first on how to refactor the UT classes.
Thanks

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

[tugsbayasgalan]

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

Hi @cccclai, @tugsbayasgalan,

Yes I am able to reproduce the issue after bumping the torch nightly version to "dev20250422".

I believe the reason is that we are trying to convert the quant weights back to floating point during

executorch/backends/qualcomm/_passes/annotate_quant_attrs.py

Line 144 in d31ef13

set_parameter(param, n.args[0], self.edge_program)

However, after the bumping the torch version, it seems like the FP weights are not properly saved.
We are using the following functions to get and set parameters.

executorch/backends/qualcomm/builders/utils.py

Line 30 in d31ef13

defget_parameter(

executorch/backends/qualcomm/builders/utils.py

Line 47 in d31ef13

defset_parameter(

It would be appreciated if you could share the best practices on handling parameters modification.
Thanks

[tugsbayasgalan]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

[winskuo-quic]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

I think I might know why it is not updating, but I need some time to verify if it is actually the root cause as I am currently blocked by something else.
I think when we are initializing the passes, we passed the exported_program into the constructor.

executorch/backends/qualcomm/_passes/qnn_pass_manager.py

Line 160 in 9e59c19

kwargs["edge_program"] =exported_program

Next, during the lowering process, when decomposition is called, we get a new exported_program with a shallow copy new_state_dict. https://github.com/pytorch/pytorch/blob/ad81eeb7c7c906e0cdd04a5cc8fdb9592281c317/torch/export/exported_program.py#L880
Then, when we are running the passes, we are updating our values to old state_dict in the old exported_program.
This is why we keep on getting incorrect params during QNN op builder.

Please let me know if there's anything that is still unclear.
Thanks

[tugsbayasgalan]

Ahh this makes sense. You should always work with state_dict of ep after running decompositions because the state dict can change after decompositions.

[tugsbayasgalan]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

[winskuo-quic]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

Hi @tugsbayasgalan,
Thanks for the suggestions.
We will work on Qualcomm code to make it compatible with new export behavior.
As you mentioned, it is probably safer to work on state dict of the ep after decomposition.

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

Sorry for the delay on pushing the PR.
We came up with couple solutions and were discussing internally which one is the best to resolve the uplevel error.
We will share the PR later today.

[cccclai]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

[winskuo-quic]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

Just rebased. Thanks for reminder

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[winskuo-quic]

Summary

Remove alias_copy op.

Test plan

Add UTs to ensure alias_copy is removed.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10319

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 4b587c0 with merge base 4e38f4a ():

NEW FAILURE - The following job has failed:

• pull / test-llava-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 92c37ffcdda88d206e2c3b3eef3e8691e272d1a47ad1f315c6d306e7b8682a1d /exec failed with exit code 139

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[winskuo-quic]

Hi @cccclai, @billmguo,
This PR is the remove alias_copy operation.
Please have a look.
Thanks

[billmguo]

Work for me

[cccclai]

Are we testing alias op should still work, meaning they're not removed?

[winskuo-quic]

Hi @cccclai,
Thanks for reviewing the PR.
Alias op will be removed.
This test is just to show alias can be properly removed and model is still working properly.
I have added some comments under the model.

[cccclai]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

[winskuo-quic]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

This test it to reproduce @billmguo error.
Without adding exir_ops.edge.aten.alias_copy.default to RemoveRedundancy pass, this test will fail during qnn_partitioner where it does not have op_builder for aten.alias_copy.default.

Thanks for the suggestion. That will probably be more straight forward as the unit test is not actually running the alias_op since it is dropped during RemoveRedundancy. Maybe we can add a new class for UT, targeting whether passes are working as expected.

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cccclai]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

[winskuo-quic]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

Thanks for sharing the results. I think this is failing by chances. I tested locally a couple times, and it all passed. I have changed to a simpler OP instead since the purpose here is to verify alias_copy is handled properly.
Would you mind if I push another PR for UT refactor to add a new class for passes related stuff later on? I think we will need to discuss internally first on how to refactor the UT classes.
Thanks

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

[tugsbayasgalan]

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

Hi @cccclai, @tugsbayasgalan,

Yes I am able to reproduce the issue after bumping the torch nightly version to "dev20250422".

I believe the reason is that we are trying to convert the quant weights back to floating point during

executorch/backends/qualcomm/_passes/annotate_quant_attrs.py

Line 144 in d31ef13

set_parameter(param, n.args[0], self.edge_program)

However, after the bumping the torch version, it seems like the FP weights are not properly saved.
We are using the following functions to get and set parameters.

executorch/backends/qualcomm/builders/utils.py

Line 30 in d31ef13

defget_parameter(

executorch/backends/qualcomm/builders/utils.py

Line 47 in d31ef13

defset_parameter(

It would be appreciated if you could share the best practices on handling parameters modification.
Thanks

[tugsbayasgalan]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

[winskuo-quic]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

I think I might know why it is not updating, but I need some time to verify if it is actually the root cause as I am currently blocked by something else.
I think when we are initializing the passes, we passed the exported_program into the constructor.

executorch/backends/qualcomm/_passes/qnn_pass_manager.py

Line 160 in 9e59c19

kwargs["edge_program"] =exported_program

Next, during the lowering process, when decomposition is called, we get a new exported_program with a shallow copy new_state_dict. https://github.com/pytorch/pytorch/blob/ad81eeb7c7c906e0cdd04a5cc8fdb9592281c317/torch/export/exported_program.py#L880
Then, when we are running the passes, we are updating our values to old state_dict in the old exported_program.
This is why we keep on getting incorrect params during QNN op builder.

Please let me know if there's anything that is still unclear.
Thanks

[tugsbayasgalan]

Ahh this makes sense. You should always work with state_dict of ep after running decompositions because the state dict can change after decompositions.

[tugsbayasgalan]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

[winskuo-quic]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

Hi @tugsbayasgalan,
Thanks for the suggestions.
We will work on Qualcomm code to make it compatible with new export behavior.
As you mentioned, it is probably safer to work on state dict of the ep after decomposition.

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

Sorry for the delay on pushing the PR.
We came up with couple solutions and were discussing internally which one is the best to resolve the uplevel error.
We will share the PR later today.

[cccclai]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

[winskuo-quic]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

Just rebased. Thanks for reminder

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[winskuo-quic]

Summary

Remove alias_copy op.

Test plan

Add UTs to ensure alias_copy is removed.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10319

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 4b587c0 with merge base 4e38f4a ():

NEW FAILURE - The following job has failed:

• pull / test-llava-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 92c37ffcdda88d206e2c3b3eef3e8691e272d1a47ad1f315c6d306e7b8682a1d /exec failed with exit code 139

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[winskuo-quic]

Hi @cccclai, @billmguo,
This PR is the remove alias_copy operation.
Please have a look.
Thanks

[billmguo]

Work for me

[cccclai]

Are we testing alias op should still work, meaning they're not removed?

[winskuo-quic]

Hi @cccclai,
Thanks for reviewing the PR.
Alias op will be removed.
This test is just to show alias can be properly removed and model is still working properly.
I have added some comments under the model.

[cccclai]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

[winskuo-quic]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

This test it to reproduce @billmguo error.
Without adding exir_ops.edge.aten.alias_copy.default to RemoveRedundancy pass, this test will fail during qnn_partitioner where it does not have op_builder for aten.alias_copy.default.

Thanks for the suggestion. That will probably be more straight forward as the unit test is not actually running the alias_op since it is dropped during RemoveRedundancy. Maybe we can add a new class for UT, targeting whether passes are working as expected.

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cccclai]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

[winskuo-quic]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

Thanks for sharing the results. I think this is failing by chances. I tested locally a couple times, and it all passed. I have changed to a simpler OP instead since the purpose here is to verify alias_copy is handled properly.
Would you mind if I push another PR for UT refactor to add a new class for passes related stuff later on? I think we will need to discuss internally first on how to refactor the UT classes.
Thanks

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

[tugsbayasgalan]

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

Hi @cccclai, @tugsbayasgalan,

Yes I am able to reproduce the issue after bumping the torch nightly version to "dev20250422".

I believe the reason is that we are trying to convert the quant weights back to floating point during

executorch/backends/qualcomm/_passes/annotate_quant_attrs.py

Line 144 in d31ef13

set_parameter(param, n.args[0], self.edge_program)

However, after the bumping the torch version, it seems like the FP weights are not properly saved.
We are using the following functions to get and set parameters.

executorch/backends/qualcomm/builders/utils.py

Line 30 in d31ef13

defget_parameter(

executorch/backends/qualcomm/builders/utils.py

Line 47 in d31ef13

defset_parameter(

It would be appreciated if you could share the best practices on handling parameters modification.
Thanks

[tugsbayasgalan]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

[winskuo-quic]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

I think I might know why it is not updating, but I need some time to verify if it is actually the root cause as I am currently blocked by something else.
I think when we are initializing the passes, we passed the exported_program into the constructor.

executorch/backends/qualcomm/_passes/qnn_pass_manager.py

Line 160 in 9e59c19

kwargs["edge_program"] =exported_program

Next, during the lowering process, when decomposition is called, we get a new exported_program with a shallow copy new_state_dict. https://github.com/pytorch/pytorch/blob/ad81eeb7c7c906e0cdd04a5cc8fdb9592281c317/torch/export/exported_program.py#L880
Then, when we are running the passes, we are updating our values to old state_dict in the old exported_program.
This is why we keep on getting incorrect params during QNN op builder.

Please let me know if there's anything that is still unclear.
Thanks

[tugsbayasgalan]

Ahh this makes sense. You should always work with state_dict of ep after running decompositions because the state dict can change after decompositions.

[tugsbayasgalan]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

[winskuo-quic]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

Hi @tugsbayasgalan,
Thanks for the suggestions.
We will work on Qualcomm code to make it compatible with new export behavior.
As you mentioned, it is probably safer to work on state dict of the ep after decomposition.

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

Sorry for the delay on pushing the PR.
We came up with couple solutions and were discussing internally which one is the best to resolve the uplevel error.
We will share the PR later today.

[cccclai]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

[winskuo-quic]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

Just rebased. Thanks for reminder

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[winskuo-quic]

Summary

Remove alias_copy op.

Test plan

Add UTs to ensure alias_copy is removed.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10319

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 4b587c0 with merge base 4e38f4a ():

NEW FAILURE - The following job has failed:

• pull / test-llava-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 92c37ffcdda88d206e2c3b3eef3e8691e272d1a47ad1f315c6d306e7b8682a1d /exec failed with exit code 139

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[winskuo-quic]

Hi @cccclai, @billmguo,
This PR is the remove alias_copy operation.
Please have a look.
Thanks

[billmguo]

Work for me

[cccclai]

Are we testing alias op should still work, meaning they're not removed?

[winskuo-quic]

Hi @cccclai,
Thanks for reviewing the PR.
Alias op will be removed.
This test is just to show alias can be properly removed and model is still working properly.
I have added some comments under the model.

[cccclai]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

[winskuo-quic]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

This test it to reproduce @billmguo error.
Without adding exir_ops.edge.aten.alias_copy.default to RemoveRedundancy pass, this test will fail during qnn_partitioner where it does not have op_builder for aten.alias_copy.default.

Thanks for the suggestion. That will probably be more straight forward as the unit test is not actually running the alias_op since it is dropped during RemoveRedundancy. Maybe we can add a new class for UT, targeting whether passes are working as expected.

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cccclai]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

[winskuo-quic]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

Thanks for sharing the results. I think this is failing by chances. I tested locally a couple times, and it all passed. I have changed to a simpler OP instead since the purpose here is to verify alias_copy is handled properly.
Would you mind if I push another PR for UT refactor to add a new class for passes related stuff later on? I think we will need to discuss internally first on how to refactor the UT classes.
Thanks

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

[tugsbayasgalan]

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

Hi @cccclai, @tugsbayasgalan,

Yes I am able to reproduce the issue after bumping the torch nightly version to "dev20250422".

I believe the reason is that we are trying to convert the quant weights back to floating point during

executorch/backends/qualcomm/_passes/annotate_quant_attrs.py

Line 144 in d31ef13

set_parameter(param, n.args[0], self.edge_program)

However, after the bumping the torch version, it seems like the FP weights are not properly saved.
We are using the following functions to get and set parameters.

executorch/backends/qualcomm/builders/utils.py

Line 30 in d31ef13

defget_parameter(

executorch/backends/qualcomm/builders/utils.py

Line 47 in d31ef13

defset_parameter(

It would be appreciated if you could share the best practices on handling parameters modification.
Thanks

[tugsbayasgalan]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

[winskuo-quic]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

I think I might know why it is not updating, but I need some time to verify if it is actually the root cause as I am currently blocked by something else.
I think when we are initializing the passes, we passed the exported_program into the constructor.

executorch/backends/qualcomm/_passes/qnn_pass_manager.py

Line 160 in 9e59c19

kwargs["edge_program"] =exported_program

Next, during the lowering process, when decomposition is called, we get a new exported_program with a shallow copy new_state_dict. https://github.com/pytorch/pytorch/blob/ad81eeb7c7c906e0cdd04a5cc8fdb9592281c317/torch/export/exported_program.py#L880
Then, when we are running the passes, we are updating our values to old state_dict in the old exported_program.
This is why we keep on getting incorrect params during QNN op builder.

Please let me know if there's anything that is still unclear.
Thanks

[tugsbayasgalan]

Ahh this makes sense. You should always work with state_dict of ep after running decompositions because the state dict can change after decompositions.

[tugsbayasgalan]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

[winskuo-quic]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

Hi @tugsbayasgalan,
Thanks for the suggestions.
We will work on Qualcomm code to make it compatible with new export behavior.
As you mentioned, it is probably safer to work on state dict of the ep after decomposition.

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

Sorry for the delay on pushing the PR.
We came up with couple solutions and were discussing internally which one is the best to resolve the uplevel error.
We will share the PR later today.

[cccclai]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

[winskuo-quic]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

Just rebased. Thanks for reminder

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[winskuo-quic]

Summary

Remove alias_copy op.

Test plan

Add UTs to ensure alias_copy is removed.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10319

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 4b587c0 with merge base 4e38f4a ():

NEW FAILURE - The following job has failed:

• pull / test-llava-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 92c37ffcdda88d206e2c3b3eef3e8691e272d1a47ad1f315c6d306e7b8682a1d /exec failed with exit code 139

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[winskuo-quic]

Hi @cccclai, @billmguo,
This PR is the remove alias_copy operation.
Please have a look.
Thanks

[billmguo]

Work for me

[cccclai]

Are we testing alias op should still work, meaning they're not removed?

[winskuo-quic]

Hi @cccclai,
Thanks for reviewing the PR.
Alias op will be removed.
This test is just to show alias can be properly removed and model is still working properly.
I have added some comments under the model.

[cccclai]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

[winskuo-quic]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

This test it to reproduce @billmguo error.
Without adding exir_ops.edge.aten.alias_copy.default to RemoveRedundancy pass, this test will fail during qnn_partitioner where it does not have op_builder for aten.alias_copy.default.

Thanks for the suggestion. That will probably be more straight forward as the unit test is not actually running the alias_op since it is dropped during RemoveRedundancy. Maybe we can add a new class for UT, targeting whether passes are working as expected.

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cccclai]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

[winskuo-quic]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

Thanks for sharing the results. I think this is failing by chances. I tested locally a couple times, and it all passed. I have changed to a simpler OP instead since the purpose here is to verify alias_copy is handled properly.
Would you mind if I push another PR for UT refactor to add a new class for passes related stuff later on? I think we will need to discuss internally first on how to refactor the UT classes.
Thanks

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

[tugsbayasgalan]

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

Hi @cccclai, @tugsbayasgalan,

Yes I am able to reproduce the issue after bumping the torch nightly version to "dev20250422".

I believe the reason is that we are trying to convert the quant weights back to floating point during

executorch/backends/qualcomm/_passes/annotate_quant_attrs.py

Line 144 in d31ef13

set_parameter(param, n.args[0], self.edge_program)

However, after the bumping the torch version, it seems like the FP weights are not properly saved.
We are using the following functions to get and set parameters.

executorch/backends/qualcomm/builders/utils.py

Line 30 in d31ef13

defget_parameter(

executorch/backends/qualcomm/builders/utils.py

Line 47 in d31ef13

defset_parameter(

It would be appreciated if you could share the best practices on handling parameters modification.
Thanks

[tugsbayasgalan]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

[winskuo-quic]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

I think I might know why it is not updating, but I need some time to verify if it is actually the root cause as I am currently blocked by something else.
I think when we are initializing the passes, we passed the exported_program into the constructor.

executorch/backends/qualcomm/_passes/qnn_pass_manager.py

Line 160 in 9e59c19

kwargs["edge_program"] =exported_program

Next, during the lowering process, when decomposition is called, we get a new exported_program with a shallow copy new_state_dict. https://github.com/pytorch/pytorch/blob/ad81eeb7c7c906e0cdd04a5cc8fdb9592281c317/torch/export/exported_program.py#L880
Then, when we are running the passes, we are updating our values to old state_dict in the old exported_program.
This is why we keep on getting incorrect params during QNN op builder.

Please let me know if there's anything that is still unclear.
Thanks

[tugsbayasgalan]

Ahh this makes sense. You should always work with state_dict of ep after running decompositions because the state dict can change after decompositions.

[tugsbayasgalan]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

[winskuo-quic]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

Hi @tugsbayasgalan,
Thanks for the suggestions.
We will work on Qualcomm code to make it compatible with new export behavior.
As you mentioned, it is probably safer to work on state dict of the ep after decomposition.

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

Sorry for the delay on pushing the PR.
We came up with couple solutions and were discussing internally which one is the best to resolve the uplevel error.
We will share the PR later today.

[cccclai]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

[winskuo-quic]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

Just rebased. Thanks for reminder

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[winskuo-quic]

Summary

Remove alias_copy op.

Test plan

Add UTs to ensure alias_copy is removed.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10319

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 4b587c0 with merge base 4e38f4a ():

NEW FAILURE - The following job has failed:

• pull / test-llava-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 92c37ffcdda88d206e2c3b3eef3e8691e272d1a47ad1f315c6d306e7b8682a1d /exec failed with exit code 139

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[winskuo-quic]

Hi @cccclai, @billmguo,
This PR is the remove alias_copy operation.
Please have a look.
Thanks

[billmguo]

Work for me

[cccclai]

Are we testing alias op should still work, meaning they're not removed?

[winskuo-quic]

Hi @cccclai,
Thanks for reviewing the PR.
Alias op will be removed.
This test is just to show alias can be properly removed and model is still working properly.
I have added some comments under the model.

[cccclai]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

[winskuo-quic]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

This test it to reproduce @billmguo error.
Without adding exir_ops.edge.aten.alias_copy.default to RemoveRedundancy pass, this test will fail during qnn_partitioner where it does not have op_builder for aten.alias_copy.default.

Thanks for the suggestion. That will probably be more straight forward as the unit test is not actually running the alias_op since it is dropped during RemoveRedundancy. Maybe we can add a new class for UT, targeting whether passes are working as expected.

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cccclai]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

[winskuo-quic]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

Thanks for sharing the results. I think this is failing by chances. I tested locally a couple times, and it all passed. I have changed to a simpler OP instead since the purpose here is to verify alias_copy is handled properly.
Would you mind if I push another PR for UT refactor to add a new class for passes related stuff later on? I think we will need to discuss internally first on how to refactor the UT classes.
Thanks

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

[tugsbayasgalan]

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

Hi @cccclai, @tugsbayasgalan,

Yes I am able to reproduce the issue after bumping the torch nightly version to "dev20250422".

I believe the reason is that we are trying to convert the quant weights back to floating point during

executorch/backends/qualcomm/_passes/annotate_quant_attrs.py

Line 144 in d31ef13

set_parameter(param, n.args[0], self.edge_program)

However, after the bumping the torch version, it seems like the FP weights are not properly saved.
We are using the following functions to get and set parameters.

executorch/backends/qualcomm/builders/utils.py

Line 30 in d31ef13

defget_parameter(

executorch/backends/qualcomm/builders/utils.py

Line 47 in d31ef13

defset_parameter(

It would be appreciated if you could share the best practices on handling parameters modification.
Thanks

[tugsbayasgalan]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

[winskuo-quic]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

I think I might know why it is not updating, but I need some time to verify if it is actually the root cause as I am currently blocked by something else.
I think when we are initializing the passes, we passed the exported_program into the constructor.

executorch/backends/qualcomm/_passes/qnn_pass_manager.py

Line 160 in 9e59c19

kwargs["edge_program"] =exported_program

Next, during the lowering process, when decomposition is called, we get a new exported_program with a shallow copy new_state_dict. https://github.com/pytorch/pytorch/blob/ad81eeb7c7c906e0cdd04a5cc8fdb9592281c317/torch/export/exported_program.py#L880
Then, when we are running the passes, we are updating our values to old state_dict in the old exported_program.
This is why we keep on getting incorrect params during QNN op builder.

Please let me know if there's anything that is still unclear.
Thanks

[tugsbayasgalan]

Ahh this makes sense. You should always work with state_dict of ep after running decompositions because the state dict can change after decompositions.

[tugsbayasgalan]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

[winskuo-quic]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

Hi @tugsbayasgalan,
Thanks for the suggestions.
We will work on Qualcomm code to make it compatible with new export behavior.
As you mentioned, it is probably safer to work on state dict of the ep after decomposition.

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

Sorry for the delay on pushing the PR.
We came up with couple solutions and were discussing internally which one is the best to resolve the uplevel error.
We will share the PR later today.

[cccclai]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

[winskuo-quic]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

Just rebased. Thanks for reminder

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[winskuo-quic]

Summary

Remove alias_copy op.

Test plan

Add UTs to ensure alias_copy is removed.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10319

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 4b587c0 with merge base 4e38f4a ():

NEW FAILURE - The following job has failed:

• pull / test-llava-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 92c37ffcdda88d206e2c3b3eef3e8691e272d1a47ad1f315c6d306e7b8682a1d /exec failed with exit code 139

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[winskuo-quic]

Hi @cccclai, @billmguo,
This PR is the remove alias_copy operation.
Please have a look.
Thanks

[billmguo]

Work for me

[cccclai]

Are we testing alias op should still work, meaning they're not removed?

[winskuo-quic]

Hi @cccclai,
Thanks for reviewing the PR.
Alias op will be removed.
This test is just to show alias can be properly removed and model is still working properly.
I have added some comments under the model.

[cccclai]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

[winskuo-quic]

hmm, I assume this test will work regardless with or without alias_op, correct? I was thinking export the graph and run to_backend, and check there is no alias op after that.

This test it to reproduce @billmguo error.
Without adding exir_ops.edge.aten.alias_copy.default to RemoveRedundancy pass, this test will fail during qnn_partitioner where it does not have op_builder for aten.alias_copy.default.

Thanks for the suggestion. That will probably be more straight forward as the unit test is not actually running the alias_op since it is dropped during RemoveRedundancy. Maybe we can add a new class for UT, targeting whether passes are working as expected.

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cccclai]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

[winskuo-quic]

It seems like the test is failing

AssertionError: False is not true : ref_output:
tensor([[ 0.5914, -0.8622, 0.0214, -0.3349, 0.0285, -0.9833, -1.2256, 0.3349,
-0.7197, 0.3278]])
model_output:
tensor([[ 0.5914, 0.5914, -1.2185, -1.2185, 0.5914, -1.2185, -1.2185, -1.2185,
-1.2185, -1.2185]])

Thanks for sharing the results. I think this is failing by chances. I tested locally a couple times, and it all passed. I have changed to a simpler OP instead since the purpose here is to verify alias_copy is handled properly.
Would you mind if I push another PR for UT refactor to add a new class for passes related stuff later on? I think we will need to discuss internally first on how to refactor the UT classes.
Thanks

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

[tugsbayasgalan]

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

I have been looking at the failure and still haven't made much progress. It looks like qualcomm passes do something special to exported artifact state dicts down the line. Is it accurate? Can you point to me places where you deal with module state dicts?

Hi @cccclai, @tugsbayasgalan,

Yes I am able to reproduce the issue after bumping the torch nightly version to "dev20250422".

I believe the reason is that we are trying to convert the quant weights back to floating point during

executorch/backends/qualcomm/_passes/annotate_quant_attrs.py

Line 144 in d31ef13

set_parameter(param, n.args[0], self.edge_program)

However, after the bumping the torch version, it seems like the FP weights are not properly saved.
We are using the following functions to get and set parameters.

executorch/backends/qualcomm/builders/utils.py

Line 30 in d31ef13

defget_parameter(

executorch/backends/qualcomm/builders/utils.py

Line 47 in d31ef13

defset_parameter(

It would be appreciated if you could share the best practices on handling parameters modification.
Thanks

[tugsbayasgalan]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

[winskuo-quic]

I am still confused how this logic could break your flow tho. Before returning state dict, i just shallow copy in export meaning they should be the same state dict lol. In what part of set_parameter does it fail?

I think I might know why it is not updating, but I need some time to verify if it is actually the root cause as I am currently blocked by something else.
I think when we are initializing the passes, we passed the exported_program into the constructor.

executorch/backends/qualcomm/_passes/qnn_pass_manager.py

Line 160 in 9e59c19

kwargs["edge_program"] =exported_program

Next, during the lowering process, when decomposition is called, we get a new exported_program with a shallow copy new_state_dict. https://github.com/pytorch/pytorch/blob/ad81eeb7c7c906e0cdd04a5cc8fdb9592281c317/torch/export/exported_program.py#L880
Then, when we are running the passes, we are updating our values to old state_dict in the old exported_program.
This is why we keep on getting incorrect params during QNN op builder.

Please let me know if there's anything that is still unclear.
Thanks

[tugsbayasgalan]

Ahh this makes sense. You should always work with state_dict of ep after running decompositions because the state dict can change after decompositions.

[tugsbayasgalan]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

[winskuo-quic]

@winskuo-quic Just checking in, will you be working on making the qualcomm code compatible with new export behaviour?

Hi @tugsbayasgalan,
Thanks for the suggestions.
We will work on Qualcomm code to make it compatible with new export behavior.
As you mentioned, it is probably safer to work on state dict of the ep after decomposition.

[cccclai]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

[winskuo-quic]

Actually the failing might due to this change #10362, can you help checking if the test is still passing with pytorch/pytorch#151436?

Any update on this change? #10362 is currently pending due to the breakage

Sorry for the delay on pushing the PR.
We came up with couple solutions and were discussing internally which one is the best to resolve the uplevel error.
We will share the PR later today.

[cccclai]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

[winskuo-quic]

Given that CI is green again, we can resume merging PRs now. Mind rebasing? This seems to be the oldest PR that needs to land

Just rebased. Thanks for reminder

[facebook-github-bot]

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.