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

Revert "Add mxnet.text APIs (#8763)" #9401

Merged
merged 1 commit into from
Jan 12, 2018
Merged

Revert "Add mxnet.text APIs (#8763)" #9401

merged 1 commit into from
Jan 12, 2018

Conversation

piiswrong
Copy link
Contributor

This reverts commit 6c1f4f7.

This is a major addition in API and needs more discussion.

Also it should wait after 1.0.1 release

Description

(Brief description on what this PR is about)

Checklist

Essentials

  • Passed code style checking (make lint)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@szha szha closed this Jan 12, 2018
@mli
Copy link
Contributor

mli commented Jan 12, 2018

how about put "experimental text api in 1.0.1", especially given it has no conflict with other modules. This PR is needed for the mooc class next week.

@szha
Copy link
Member

szha commented Jan 12, 2018

Let's have the discussion happen first.

@piiswrong piiswrong reopened this Jan 12, 2018
@mli
Copy link
Contributor

mli commented Jan 12, 2018

I think @piiswrong 's concern is that if we put this module into mxnet.text, then if after one year we learned new things from using it, then it could be hard to change it. Given it is totally, the chance to change the API is hard.

So one way is rename mxnet.text to mxnet.contrib.text. After this module is in a better shape, namely having more functionalities, having been used by others projects. Then we can review it and move back to mxnet.text

@piiswrong If this is what you thought. I believe instead of reverting this PR, a better way to communicate is

  1. post this discussion
  2. submit a rename PR

@piiswrong
Copy link
Contributor Author

This, plus there are naming issues that needs more review.
For example Glossary is weird here. It's usually called Vocabulary.
So I decided to rollback and have the original author resubmit.

Plus rolling back is usually better than fixing forward. We should rollback this as soon as possible before someone starts depending on it.

@piiswrong
Copy link
Contributor Author

@astonzhang Please resubmit to contrib.

@szha
Copy link
Member

szha commented Jan 12, 2018

@piiswrong agreed to move to contrib. I will merge this PR and resubmit.

@szha szha merged commit 67cfbc8 into apache:master Jan 12, 2018
CodingCat pushed a commit to CodingCat/mxnet that referenced this pull request Jan 16, 2018
yuxiangw pushed a commit to yuxiangw/incubator-mxnet that referenced this pull request Jan 25, 2018
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants