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

[MXNET-908] Enable python tests in Travis #12550

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

KellenSunderland
Copy link
Contributor

Description

This turns on python tests in Travis in order to increase or MacOS coverage.

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)
  • 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
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@kalyc
Copy link
Contributor

kalyc commented Sep 14, 2018

Thanks for your contribution @KellenSunderland
I noticed the build is failing
@mxnet-label-bot[pr-awaiting-response]

@marcoabreu marcoabreu added the pr-awaiting-response PR is reviewed and waiting for contributor to respond label Sep 14, 2018
@KellenSunderland
Copy link
Contributor Author

KellenSunderland commented Sep 15, 2018 via email

@marcoabreu marcoabreu added pr-work-in-progress PR is still work in progress and removed pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Sep 17, 2018
@marcoabreu
Copy link
Contributor

@KellenSunderland it seems like everything is passing. Is this good to merge?

@KellenSunderland
Copy link
Contributor Author

KellenSunderland commented Sep 17, 2018 via email

@@ -29,3 +29,4 @@ script:
- export MXNET_STORAGE_FALLBACK_LOG_VERBOSE=0
- mv make/osx.mk config.mk
- make -j 2
Copy link
Member

Choose a reason for hiding this comment

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

@KellenSunderland instead of placing python here as a job that travis would always execute, could you please put python build section into a separate target so we can also add Scala-maven test in here in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be great if we could add Scala tests as well, but I do have a few concerns. How long would they take to execute? Let's keep in mind that we have a limited number of executors that can be run before we begin interfering with other Apache projects. There's also a global timeout of ~45 minutes to build and execute on a virtualized MacOS host which takes a few minutes just to start. I'd ideally like to keep our total test time below 30 minutes to avoid hitting this in the future.

In principle I'm not against build stages, but unless we have a lot of jobs running in parallel that have dependencies on each other I don't see a big benefit in using them.

Copy link
Member

@lanking520 lanking520 Sep 18, 2018

Choose a reason for hiding this comment

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

Thanks for your reply! If you mean scalaunittestit will be around 10 mins or less. Is travis wait possible to apply in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately Travis wait won't help in this case. The tests are active and sending output to Travis, they just take a long time and hit the global timeout for Travis jobs.

10 minutes actually is reasonable. Let's do a test run with Scala enabled after this is merged and see if we can get it included. My guess is if we enable folder caching, and cache models we're currently downloading we'll be able to fit it in and not hit the global timeouts.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, let's try to make Scala test available later in Travis.

@KellenSunderland
Copy link
Contributor Author

Note: this will fail in Travis until this PR is merged: #12590

This turns on python tests in Travis in order to increase or MacOS coverage.
@KellenSunderland
Copy link
Contributor Author

Rebased, should build properly now.

@marcoabreu marcoabreu merged commit 61b17b7 into apache:master Sep 19, 2018
@lanking520
Copy link
Member

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants