Hi Lin, Thanks for the suggestions.
With respect to your proposal: > (2) Any PR that contains API change should clearly state this in PR title. > Otherwise, committer can reject the PR I agree that PRs with API changes should be made more prominent. Another mechanism that has already been used is to tag the PRs with the "API change" label [1]. On the other hand, relying on the community to call out the PRs with API changes may not be reliable. Oftentimes, people didn't realize that a change constitutes an API change. If a committer identifies such a change, a more friendly response would be to just label the PR and call out where the API change happens in a comment. > (1) Any PR involving change of APIs should be approved by at least one of > the committers from a "API Committee" before it can be merged. I will > explain how to form this committee at the end of email I'm not convinced that more hierarchy should be created among committers. All committers are entrusted by the PPMC to use their judgement to the best interest of this project, and additional qualification seems counter-productive. With respect to your linked issue #17292, as @stephenrawls pointed out, it comes from 4ed14e2 where the inline definition of MXAPIHandleException is moved to a .cc file, and I'm the one that actually made this change to unblock the PR. I want to call out that: - This is not an API change in that there's no change in the function signature or visibility in the symbol table of libmxnet.so. - It should not be the responsibility of MXNet to maintain the assumption that downstream projects like horovod make in their building logic. A more pressing issue should have been the way that a third-party communication library like horovod integrates with MXNet. So far the horovod integration seemed brittle and there have been many issues [2]. For this specific issue, to me, it doesn't seem like a good decision on the horovod side to assume any function would be defined inline on the MXNet side. With the development of MXNet 2.0, it's a good time to rethink how horovod integration should work with MXNet. I'm hoping that MXNet 2.0 item 4.11 AbstractKVStore interface (See #17115) could help simplify and alleviate the coupling in the current way of integration. -sz [1] https://github.com/apache/incubator-mxnet/pulls?utf8=%E2%9C%93&q=is%3Apr+label%3A%22API+change%22+ [2] https://github.com/apache/incubator-mxnet/issues?utf8=%E2%9C%93&q=is%3Aissue+horovod On 2020/01/14 00:37:55, Lin Yuan <apefor...@gmail.com> wrote: > Dear Community, > > Recently, there were some changes to C APIs that broke another downstream > project Horovod: https://github.com/apache/incubator-mxnet/issues/17292. > Since we do not have integration tests for downstream project, it becomes > critical for us to update APIs with extreme caution. > > I would like to propose the following mechanism for us to maintain a clean > and robust APIs: including both C API and Python API at the moment. > > (1) Any PR involving change of APIs should be approved by at least one of > the committers from a "API Committee" before it can be merged. I will > explain how to form this committee at the end of email > > (2) Any PR that contains API change should clearly state this in PR title. > Otherwise, committer can reject the PR > > API Committee: > - This committee should consist of both seasoned MXNet developers and users. > - Committee members should have a comprehensive view of MXNet APIs to make > sure their usage are consistent across stack. > - Committee members review PRs that involve API change with extra caution. > - Committee members are required to attend the roadmap discussion for each > new release. > - For API breaking changes, committe members should reach consensus before > the change is made. > > Any other suggestion is welcome here. > > Best, > > Lin >
