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

Backporting recent mx.np changes to 1.7 branch #18641

Open
leezu opened this issue Jun 30, 2020 · 14 comments
Open

Backporting recent mx.np changes to 1.7 branch #18641

leezu opened this issue Jun 30, 2020 · 14 comments
Labels
Bug v1.x Targeting v1.x branch

Comments

@leezu
Copy link
Contributor

leezu commented Jun 30, 2020

Description

master branch contains many fixes for various numpy operators included in the 1.7 release. While they are experimental in 1.7, it would be helpful to include the fixes. For example, I tested running GluonNLP NumPy version with the 1.7 branch and some unittests are failing.

For example, MXNetError: Operator _npi_unique is non-differentiable because it didn't register FGradient attribute which was fixed in #18074 is not part of 1.7

To fix this issue, one could copy over all numpy operator related C++ and Python files from the master branch to the 1.x and 1.7 branches.

https://github.com/leezu/gluon-nlp/runs/820282077?check_suite_focus=true

cc: @ciyongch @sxjscience @yzhliu

@ciyongch
Copy link
Contributor

Hi @leezu, given the fact that the current numpy operator is still in active development, there could be more defects/bugs as including more new functionalities/features in v1.7. Thus it's uncertain about how longer it will take to backport these numpy bug fixes/features from master to v1.7, I suggest to mark numpy operator as experimental feature in v1.7 release, and decide a cut off day (for example 24h or 48h) to include the fixes that are available, and moving the 1.7 release process forward, what do you think?
@szha @pengzhao-intel @TaoLv @ChaiBapchya @sandeep-krishnamurthy

@sandeep-krishnamurthy
Copy link
Contributor

+1 to mark this as experimental as it was not stable in 1.6 or advertised as will be stable in v1.7. We are mainly looking at numpy ops in 2.0. What do you think @leezu and @szha

@sxjscience
Copy link
Member

@sandeep-krishnamurthy @ciyongch How about to port these two bug-fixes to 1.7?

@szha
Copy link
Member

szha commented Jun 30, 2020

@sandeep-krishnamurthy @ciyongch I agree on marking numpy as experimental. Still, we'd really like the bug fixes that @sxjscience requests backported, so that we could use the related functionality and present them at the upcoming KDD in the summer.

@ciyongch
Copy link
Contributor

Thanks @sandeep-krishnamurthy @szha , then let's take it as the experimental feature in v1.7 release. @sxjscience could you please help to backport these two PRs and tag me on the new PR? Then we'll move forward with rc0 tag and the rest of release process when they're get merged.

@ciyongch
Copy link
Contributor

ciyongch commented Jul 1, 2020

Hi @sxjscience may I know if you're going to backport the above two PR into 1.7 as @szha suggested? We're waiting for them to be merged and tag rc0 now, thanks!

@sxjscience
Copy link
Member

@ciyongch I'm initiating the attempt. I'm able to backport #18080. But #18523 has some conflict.

@ciyongch
Copy link
Contributor

ciyongch commented Jul 1, 2020

Thanks @sxjscience for your prompt help on this. I've already ping the author for #18523, do you need any help for this PR?

@sxjscience
Copy link
Member

@ciyongch The problem is that I think there are more numpy PRs that should may need to be backported...

@ciyongch
Copy link
Contributor

ciyongch commented Jul 1, 2020

Wow... can you help to evaluate how many PRs are related to enable this feature? Then we can decide the time needed and whether to include them or not? Thanks!

@sxjscience
Copy link
Member

sxjscience commented Jul 1, 2020 via email

@ciyongch
Copy link
Contributor

ciyongch commented Jul 6, 2020

Thanks @sxjscience and @BenjaminCHEN2016 for the great effort to backport the requested fixes to v1.7, now all of them were merged into v1.7.x via the PR #18653 and #18649. I will proceed with rc0 when the night build test passed.

@ciyongch
Copy link
Contributor

Should we close this issue as the minimum fixes were backport to v1.7 release. Or we can make a complete fix in the upcoming 1.8 release? @leezu @samskalicky

@samskalicky
Copy link
Contributor

Yes, if all the fixes are in v1.x we can close this issue. Given where we are in the release process for v1.8 it would be preferred to have these fixes in the v1.8 so we can have a complete fix in the v1.8 release.

But if not, we can still close the issue as long as its fixed in v1.x and will make it into the next release (ie. v1.9)

@leezu leezu added v1.x Targeting v1.x branch and removed R1.7.0 labels Sep 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug v1.x Targeting v1.x branch
Projects
None yet
Development

No branches or pull requests

6 participants