-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: Travis building on container-based infrastructure #12946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
does this cause anything to fail (on all runs)? e.g. so it seems that we are NOT testing the locale stuff then ? (w/o the sudo) |
These are all the runs of a clean fork running on containers https://travis-ci.org/nparley/pandas/builds/124695150 (vs not https://travis-ci.org/nparley/pandas/builds/124778490). Most of the runs pass fine it seems.
|
you may need to change the |
Would the preference be to try and get travis working with containers over setting to sudo required? |
IIUC travis migrated us to a 'new' container infrastructure, but we are still doing it the 'old' way? is that accurate (IOW, the old warning has disappeared but they are doing some sort of magic to still put us on the containers). or is that completely wrong? ideally we want to be doing this the most preferred way on travis. we don't care too much are actually using sudo as eveything lives in a conda env anyhow. |
From what I understand if you were using travis before 2015-01-01 you are still using standard infrastructure.
(from https://docs.travis-ci.com/user/workers/container-based-infrastructure/) When you are running on containers you get the message:
which does not appear for |
so we are on gce. is that the container? |
No they are just ubuntu VMs on gce I think. It's a container if it has docker worker:
They changed their VMs to gce as well as adding containers and then it was a bit confusing if you were being migrated to gce or containers. |
@nparley ok I guess we are not on containers. I would like to change. The main reason is that we can then use Travis own caching system, rather then our out-sourced version (with |
@nparley does this now run on containers? can you rebase and repush |
@jreback no this PR actually is just forcing Travis not to run on containers to make sure it builds. I have just moved house the last two weeks so haven't had time to look at it again but will have a go at seeing if the locale stuff can be done at containers soon. |
@@ -12,7 +12,8 @@ pip uninstall numpy -y | |||
# these wheels don't play nice with the conda libgfortran / openblas | |||
# time conda install -n pandas libgfortran openblas || exit 1 | |||
|
|||
time sudo apt-get $APT_ARGS install libatlas-base-dev gfortran | |||
# Not going to work in containers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when you get this to work, you can just delete these lines. a comment is fine too, but don't want people uncommenting it unless they really understand what is going on (so put the comment in .travis.yml where it will be paid a bit more attention).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I invent or find a way of doing something like:
if not in container:
sudo install package
or is it better to just remove all sudos from the travis scripts?
This is now building on containers ok, https://travis-ci.org/nparley/pandas/builds/128974759. I changed the local override to zh_CN.UTF-8 from zh_CN.GB18030. The PR needs a clean up and a squash I think. The thing I have not looked at yet is the caching part. |
@@ -82,13 +102,22 @@ matrix: | |||
- JOB_TAG=_NUMPY_DEV | |||
- NOSE_ARGS="not slow and not network and not disabled" | |||
- PANDAS_TESTING_MODE="deprecate" | |||
addons: | |||
apt: | |||
packages: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so some of the builds are showing up above the allow_failures
section. We want to keep the first 5 builds (osx, 2 x 2.7, 3.4, 3.5) as the required builds, the rest as allow_failures (mainly so things don't take so long, not because they are actually allowed to fail).
So you need to repeat anything you added on in the declaration as below; e.g. for the NUMPY_DEV build, you added:
addons:
apt:
packages:
...
just repeat this in the below section (so they are identical), then it will be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this, it was caused by not indenting the new section correcting in the travis file in the allow failures section.
can we print the local in
I think this should print on the 2 non-default locales. |
Current coverage is 84.33%@@ master #12946 diff @@
==========================================
Files 138 138
Lines 51106 51107 +1
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 43102 43103 +1
Misses 8004 8004
Partials 0 0
|
BLD: correctly install tools.tests.data closes #13472
@jreback Although it's not needed to be safe I have added some code that will not use the cython cached files if a PR has changed any of the cython files. Do you think this is a good plan? For a normal branch it will not use the cython cache if the last two commits have any cython files changes. The other safeguard is to clear caches if anything in ci/ has changed in the last two commits. I have one more merge to be up to date with master. |
@jorisvandenbossche can you have a look |
- JOB_TAG=_DOC_BUILD | ||
- JOB_NAME: "doc_build" | ||
- FULL_DEPS=true | ||
- DOC_BUILD=true # if rst files were changed, build docs in parallel with tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove this comment its old
@nparley can you point to travis runs that:
|
Something is still not quite correct in the PR which needs a bit of investigating. On my travis: https://travis-ci.org/nparley/pandas/jobs/138912152 is a build that used cache. The build before https://travis-ci.org/nparley/pandas/jobs/138892021 force removed the cache before building. |
…e travis.yml comment
OK I have got the branch and pull request working correctly now. For the last commit (6c05a13): https://travis-ci.org/pydata/pandas/builds/139170756 - Are the PR travis builds In Not a PR: checking for changes in ci/ from last 2 commits
3 2 ci/check_cache.sh
2 2 ci/prep_cython_cache.sh
Files have changed in ci/ deleting all caches and from PR build: PR: checking for changes in ci/ from last 2 commits
From https://github.com/pydata/pandas
* [new ref] refs/pull/12946/head -> PR_HEAD
3 2 ci/check_cache.sh
2 2 ci/prep_cython_cache.sh
Files have changed in ci/ deleting all caches As ci files were changed in the commit before this one the cache will be deleted and a fresh build will be used. This can be seen in ls: cannot access /home/travis/.cache/: No such file or directory
Rebuilding cythonized files I.e. there is nothing in Using clean Miniconda install
Not using ccache and also checks can be made to check files are been cythoned, e.g.
and ccache is not being used, e.g.
The next commit should in theory now use the cache as long as no files in ci are hit. I push the next merge now and test this and comment on the results... (The problem was the travis checks out a detached head of the PR merge and so the |
The next commit bbc7b35 has not had any files changed in ci. So we should now be able to use cache. In check cache we have: Not a PR: checking for changes in ci/ from last 2 commits and PR: checking for changes in ci/ from last 2 commits
From https://github.com/pydata/pandas
* [new ref] refs/pull/12946/head -> PR_HEAD I.e. nothing changed in ci. So the cache has not been deleted. However in cython_files.tar motd.legal-displayed pip
Cache available
Not a PR: checking for cython files changes from last 2 commits
1 3 pandas/tslib.pyx
number of cython files changed: 1
Rebuilding cythonized files
Use cache = true
Clear cache = 1 and cython_files.tar motd.legal-displayed pip
Cache available
PR: checking for any cython file changes from last 5 commits
1 3 pandas/tslib.pyx
number of cython files changed: 1
Rebuilding cythonized files
Use cache = true
Clear cache = 1 In Miniconda install already present from cache: /home/travis/miniconda
update conda
Fetching package metadata .......
# All requested packages already installed.
# packages in environment at /home/travis/miniconda:
#
...
Using ccache
gcc: /usr/lib/ccache/gcc
ccache: /usr/bin/ccache
...
ccache gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC but also we can see that the cython cache is indeed not been being used: cythoning pandas/index.pyx to pandas/index.c Now we need another two commits that I can merge that don't have a cython change. To test that.. |
#!/bin/bash | ||
|
||
if [ "$TRAVIS_PULL_REQUEST" == "false" ] | ||
then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clever!
ok @nparley this looks really good. @jorisvandenbossche just have a look. when everyone satisfied we can merge. Then will watch for a little bit to make sure actually working. |
@jreback There is a test failing which is not on the master builds. The local override is set to
It's coming from this bit of code: def test_constructor_bad_file(self):
if is_platform_windows():
raise nose.SkipTest("skipping construction error messages "
"tests on windows")
non_file = StringIO('I am not a file')
non_file.fileno = lambda: -1
msg = "Invalid argument"
tm.assertRaisesRegexp(mmap.error, msg, common.MMapWrapper, non_file) Argomento non valido is Italian for Invalid argument, but the question is why this is not currently being picked up? |
I am not that familiar with travis details, but generally looks good. Thanks for all your work on this! |
ok, so @nparley can you squash everything down. ping when ready to merge. |
@jreback I have made another PR to fix the build error with mmap and the Italian error message here: #13507. Maybe this should be fixed before merge so all the builds will pass on merge. This branch tested with this build https://travis-ci.org/nparley/pandas/builds/139893325 is this PR merged with PR13507. |
Fixes a build error from #12946 caused by mmap error being returned in Italian when `LOCALE_OVERRIDE="it_IT.UTF-8"`. The test fails with: `AssertionError: "Invalid argument" does not match "[Errno 22] Argomento non valido"` ```python msg = "Invalid argument" tm.assertRaisesRegexp(mmap.error, msg, common.MMapWrapper, non_file) ``` i.e. message is not being matched. Change to match the errno instead as that's the same across languages. Author: Neil Parley <[email protected]> Closes #13507 from nparley/mmap-test-fix and squashes the following commits: 160af24 [Neil Parley] mmap error is not always returned in English
thanks @nparley tremendous effort! keep on the lookout for build the next few days and give a period check to make sure things still look ok. thanks again! |
Great stuff @nparley! |
git diff upstream/master | flake8 --diff
Travis is not always detecting sudo use and is running forks using container-based infrastructure.
JOB_NAME=27_slow_nnet_LOCALE
fails when using containers because of setlocale. This PR addssudo: required
to make sure travis does not use containers while sudo is needed in the travis set up scripts.Example: