[ONNX] Add OpSet 13 implementation for Hardmax

[michalpiszczek]

Add OpSet 13 Hardmax impl

[mbrookhart]

LGTM. cc @masahi and @jwfromm for thoughts on mypy

[AndrewZhaoLuo]

Please separate the Hardmax changes from the typing work in a different PR.

[jwfromm]

Since the hardmax change itself is very small (and looks good) I'm fine having it combined with typing support, which I also consider a good change. Thanks Michal!

[AndrewZhaoLuo]

I mean I understand having to make small changes in order to support hardmax. But these other changes don't support hardmax implementation and can easily be another PR.

It's important to have some PR sanitation when it's easy to do so otherwise no one will do it when it's hard (and actually needed)

[jwfromm]

I agree it would be better behavior to separate into two PRs. @michalpiszczek since the type annotation is larger change for this PR, can you separate out the hardmax changes into a different PR?

[michalpiszczek]

Thanks for the comments @AndrewZhaoLuo @jwfromm ! I am happy to take out the typing changes if that's what it takes to get Hardmax in, but I would share a different perspective first based on a previous experience.

At $prevjob I worked on a large python repo that was also initially untyped (but wanted to eventually be fully-typed) and we found that the best way to do this was for devs to add typings in and around old code as they went, rather than making separate, self-contained efforts to add types. We learned that the latter was difficult to rely upon because, with feature work and other responsibilities, finding dedicated time to go and fix up old files just didn't happen. On the other hand, embracing the first approach ended up creating a culture of leaving files you touch better than you found them amongst developers, which more organically distributed the burden of adding types. Gradually, that code base was able to become fully-typed.

I would suggest we take the same approach here, particularly because TVM's CI duration adds a non-trivial time/attention cost to making any change, further discouraging standalone cleanups.

Please let me know WYT @AndrewZhaoLuo @jwfromm . Thanks! 🙂

[AndrewZhaoLuo]

TVM is a large project with many different contributors and we want to make things like reading the git log simple. Externally we want to encourage people to have good PR hygiene. Now for an internal repo that a select few people are working on it's probably fine to do whatever internally they agree upon. Now if you submitted an RFC which suggested this doctrine I would of course be amenable.

As for TVM CI duration, I understand you want to get these changes in as fast as possible, but unless this is actively unblocking someone or CI I do not see this change as important enough to warrant a rushed job. Anyways, we want discussion on the typing changes and ideally we want those threads to be separate from the hardmax changes.

Here is what I suggest. Rewind this PR back to 6a0041d and submit the branch in it's current state as a separate PR. Require merging this PR first before the other. You will have to rebase if new ONNX changes come in, but this provides you an opportunity to add type annotations you would need to add anyway.

[mbaret]

Just to add another comment in favour of splitting the PRs, if we have patches that fix/cause issues we may very well want to revert them or cherry pick them onto older releases. In those cases, we don't want typing fixes to be associated with functional changes. I still think it's a good intiative to update the typing as you work in a particular area, but those typing changes can land in a separate PR.

[michalpiszczek]

Annotations removed. Thanks for the comments @AndrewZhaoLuo @mbaret .

[AndrewZhaoLuo]

LGTM

[AndrewZhaoLuo]

Please edit the title though before merging