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

Fix pea.sparse on IPU #16

Merged
merged 3 commits into from
Apr 28, 2023
Merged

Fix pea.sparse on IPU #16

merged 3 commits into from
Apr 28, 2023

Conversation

DouglasOrr
Copy link
Contributor

Addresses two issues:

  1. The example script breaks with No default opset version defined for domain 'ai.graphcore.pea'. This seems to be happening since the 3.2 update, and does not occur in tests. Running under gdb, seems to be triggered by shape inference and the non-extensibility of PopART's Ir::getDefaultOpsetVersion. I'm not sure if this might hit other custom ops using the "ai.graphcore.pea" domain, or if there is a better workaround than just reverting to "ai.graphcore".
  2. A known issue - since spmm does not add its' attributes under appendAttributes/appendOutlineAttributes, we get spurious cache hits "every SpMM is the same".

Full error:

2023-04-24T11:03:37.971078Z popart:popart 50817.50817 E: No default opset version defined for domain 'ai.graphcore.pea'

[0] popart::OpManager::createOpInGraph(onnx::NodeProto const&, popart::Graph&)
[1] popart::Graph::constructFromOnnxGraph(onnx::GraphProto const&)

@DouglasOrr
Copy link
Contributor Author

(Multiple reviewers - just for whoever gets there first; cheers!)

@AlCatt91
Copy link
Contributor

AlCatt91 commented Apr 27, 2023

I believe I once came across the No default opset version defined for domain 'ai.graphcore.pea' error in 3.2 using the distance_matrix custom op, however at the time I dismissed it since there were also other bugs involved in my script and I thought it was a consequence of them... However, from what you report it might actually be an issue: I think at this point it might be safer to use the ai.graphcore domain everywhere in PEA.

@DouglasOrr
Copy link
Contributor Author

Good point, thanks for the heads up! I've pushed a change to use ai.graphcore everywhere (there should be no downsides to doing so, as a name collision is unlikely, and could be worked around in any case.)

Copy link
Contributor

@AlCatt91 AlCatt91 left a comment

Choose a reason for hiding this comment

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

Thanks for updating the other ops too, I agree this should not cause major issues in terms of collisions. Looks good to me!

@DouglasOrr DouglasOrr merged commit 08311ee into main Apr 28, 2023
@DouglasOrr DouglasOrr deleted the fix-spmm branch April 28, 2023 09:13
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.

None yet

2 participants