Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[1.x] Backport #19103 #19117

Merged
merged 3 commits into from
Sep 14, 2020
Merged

[1.x] Backport #19103 #19117

merged 3 commits into from
Sep 14, 2020

Conversation

samskalicky
Copy link
Contributor

Description

Backport #19103

@mxnet-bot
Copy link

Hey @samskalicky , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [website, centos-gpu, edge, unix-cpu, clang, windows-cpu, windows-gpu, unix-gpu, centos-cpu, miscellaneous, sanity]


Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@samskalicky samskalicky added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Sep 13, 2020
szha
szha previously requested changes Sep 13, 2020
Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

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

MXNet follows semantic versioning so we can't simply change the signature of C API

Comment on lines +735 to +737
const MXContext& ctx,
const std::vector<std::vector<unsigned int> >& in_shapes,
const std::vector<int> in_types,
Copy link
Member

Choose a reason for hiding this comment

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

this is an API change from 1.x

Copy link
Contributor Author

@samskalicky samskalicky Sep 13, 2020

Choose a reason for hiding this comment

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

Hi @szha, youre right. The Extensions API is not backwards compatible. Every change increments the version number:
https://github.com/apache/incubator-mxnet/blob/f1acda73e0b2ca5f1b6ff89f25c17a838819be82/include/mxnet/lib_api.h#L56
And the version number is required to match:
https://github.com/apache/incubator-mxnet/blob/f1acda73e0b2ca5f1b6ff89f25c17a838819be82/src/c_api/c_api.cc#L1500-L1503
Each release of MXNet will require libraries to have the same version of lib_api.h. The versioning of the Extensions APIs is different than MXNet's C API.

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-awaiting-merge Review and CI is complete. Ready to Merge pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review labels Sep 13, 2020
@szha szha dismissed their stale review September 13, 2020 04:27

separate versioning

@samskalicky
Copy link
Contributor Author

@szha thanks for the catch! looks like we missed upgrading the version number in #19103 on master. I'll open a PR to increment it there too.

@samskalicky
Copy link
Contributor Author

Separately, we should restart the versioning discussion. @rondogency had some thoughts on that, but we pushed it off. We figured there would be a bunch of PRs with fixes/enhancements/etc that would be hard to maintain compatibility during the initial extensions work. It would be great if we had a plan for backwards compatibility with the extensions for 2.0. Or at least the APIs are more mature now so maybe we wont need API changes (as often :-D)

@samskalicky
Copy link
Contributor Author

Opened #19134 to update version on master

@lanking520 lanking520 added pr-awaiting-review PR is waiting for code review and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Sep 13, 2020
@szha
Copy link
Member

szha commented Sep 13, 2020

@samskalicky we concluded that 2.0 does not need to guarantee backward compatibility in general (see #17676). It would be up to the author of individual features to decide whether to backport these features.

@samskalicky
Copy link
Contributor Author

@samskalicky we concluded that 2.0 does not need to guarantee backward compatibility in general (see #17676). It would be up to the author of individual features to decide whether to backport these features.

Thanks, good to know for 1.x to 2.0. But I was thinking starting at 2.0 onward we should think about backwards compatibility (2.1, 2.2, etc).

@szha
Copy link
Member

szha commented Sep 13, 2020

@samskalicky we concluded that 2.0 does not need to guarantee backward compatibility in general (see #17676). It would be up to the author of individual features to decide whether to backport these features.

Thanks, good to know for 1.x to 2.0. But I was thinking starting at 2.0 onward we should think about backwards compatibility (2.1, 2.2, etc).

The community discussed and agreed that MXNet follows semantic versioning, which should already cover what you mentioned. If there's any aspect of it that needs revisiting, let's create a new discussion thread. https://lists.apache.org/thread.html/c52f8353f63c1e63b2646ec3b08d9a8180a1381787d777b41b8ac69f%40%3Cdev.mxnet.apache.org%3E

@samskalicky
Copy link
Contributor Author

@samskalicky we concluded that 2.0 does not need to guarantee backward compatibility in general (see #17676). It would be up to the author of individual features to decide whether to backport these features.

Thanks, good to know for 1.x to 2.0. But I was thinking starting at 2.0 onward we should think about backwards compatibility (2.1, 2.2, etc).

The community discussed and agreed that MXNet follows semantic versioning, which should already cover what you mentioned. If there's any aspect of it that needs revisiting, let's create a new discussion thread. https://lists.apache.org/thread.html/c52f8353f63c1e63b2646ec3b08d9a8180a1381787d777b41b8ac69f%40%3Cdev.mxnet.apache.org%3E

The problem I was referring to was not whether we should do versioning or not, but how to implement it. Specifically, ensuring old stuff works in new versions. Plus checking for forward incompatibility, ensuring we can identify when a newer version of a library is used in an older version of MXNet.

I think the biggest problem is coming up with a way to test/validate this. It will be too much work to maintain a set of libraries for each API across every version and test all combinations. For sure, the easiest solution is to continue with the current approach where we require the versions to match.

@szha
Copy link
Member

szha commented Sep 13, 2020

ensuring old stuff works in new versions

This is done by not allowing backward incompatible changes. It's ensured by not allowing any breaking API changes and reverting any if found.

Thinking in this line, I'm really not sure if a separate versioning for the operator extension is the right way to go, given that the API appears in C API.

Plus checking for forward incompatibility

I don't think semantic versioning covers forward compatibility, so this will require a separate discussion. I don't think MXNet is ready for supporting both backward compatibility and forward compatibility and I'm yet to see a case for requiring the latter.

@samskalicky
Copy link
Contributor Author

ensuring old stuff works in new versions

This is done by not allowing backward incompatible changes. It's ensured by not allowing any breaking API changes and reverting any if found.

Thinking in this line, I'm really not sure if a separate versioning for the operator extension is the right way to go, given that the API appears in C API.

Plus checking for forward incompatibility

I don't think semantic versioning covers forward compatibility, so this will require a separate discussion. I don't think MXNet is ready for supporting both backward compatibility and forward compatibility and I'm yet to see a case for requiring the latter.

Ok, I opened another issue to discuss this further #19135. Thanks!

@samskalicky samskalicky merged commit 9383282 into apache:v1.x Sep 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants