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

review require() usages to add meaningful messages. #12570

Merged
merged 2 commits into from
Sep 20, 2018

Conversation

mdespriee
Copy link
Contributor

@mdespriee mdespriee commented Sep 14, 2018

Description

The motivation of this PR is to add & improve error messages raised by failed requirements.
In relation to #12565.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)

Changes

No feature change.

Comment

Unfortunately, i'm not sure to really improve the error messages regarding shape inference. Real improvement would come from backend error messages. Any suggestions ?

@kalyc
Copy link
Contributor

kalyc commented Sep 15, 2018

Thanks for your contribution @mdespriee
@mxnet-label-bot[pr-work-in-progress]

@marcoabreu marcoabreu added the pr-work-in-progress PR is still work in progress label Sep 15, 2018
@nswamy
Copy link
Member

nswamy commented Sep 15, 2018

@mdespriee thanks a lot for taking the time to improve the messages. there are a few scalastyle errors, could you please fix them, test locally(at least the unit tests) and then push. I can merge the code later

@mdespriee mdespriee changed the title [WIP] review require() usages to add meaningful messages. review require() usages to add meaningful messages. Sep 16, 2018
@mdespriee
Copy link
Contributor Author

Removing [WIP]. The failing test is probably a flaky test (clojure).

@lanking520
Copy link
Member

@mdespriee please fix this line:

error file=/work/mxnet/scala-package/core/src/main/scala/org/apache/mxnet/Symbol.scala message=File line length exceeds 100 characters line=1099

You should be able to pass them afterwards

@andrewfayres
Copy link
Contributor

Mostly just commenting so I can easily watch this but I really like this improvement!

@mdespriee
Copy link
Contributor Author

@lanking520 fixed. Any existing JIRA to mention ?

@nswamy nswamy merged commit 3401e6e into apache:master Sep 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-work-in-progress PR is still work in progress Scala
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants