Skip to content
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

CI: unify environment creation #24632

Merged
merged 29 commits into from
Apr 5, 2019
Merged

CI: unify environment creation #24632

merged 29 commits into from
Apr 5, 2019

Conversation

saurav2608
Copy link

@saurav2608 saurav2608 commented Jan 5, 2019

closes #24498
closes #23923

@codecov
Copy link

codecov bot commented Jan 5, 2019

Codecov Report

Merging #24632 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24632      +/-   ##
==========================================
- Coverage   92.38%   92.37%   -0.01%     
==========================================
  Files         166      166              
  Lines       52395    52395              
==========================================
- Hits        48403    48402       -1     
- Misses       3992     3993       +1
Flag Coverage Δ
#multiple 90.8% <ø> (ø) ⬆️
#single 43% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/util/testing.py 88% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb31b2b...a8d22f3. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 5, 2019

Codecov Report

Merging #24632 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24632      +/-   ##
==========================================
- Coverage   91.81%   91.77%   -0.05%     
==========================================
  Files         175      175              
  Lines       52580    52580              
==========================================
- Hits        48278    48255      -23     
- Misses       4302     4325      +23
Flag Coverage Δ
#multiple 90.33% <ø> (-0.04%) ⬇️
#single 40.72% <ø> (-1.26%) ⬇️
Impacted Files Coverage Δ
pandas/io/clipboard/__init__.py 39.21% <0%> (-17.65%) ⬇️
pandas/io/gbq.py 75% <0%> (-12.5%) ⬇️
pandas/io/clipboard/clipboards.py 18.51% <0%> (-12.35%) ⬇️
pandas/core/frame.py 96.79% <0%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de3a85c...e400387. Read the comment docs.

@saurav2608
Copy link
Author

This sets up DB in both travis and azure. Have not tested DB jobs on Azure but maybe worth a try.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

needs some edits

ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
@jreback jreback added the CI Continuous Integration label Jan 5, 2019
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
@jreback
Copy link
Contributor

jreback commented Jan 5, 2019

yes remove the original files

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

besides the comments, there is an inconsistent use of the echo commands, with cases like:

  • echo Error
  • echo "done"
  • echo "[done]"

And some times with a bare echo before and some without (I guess those add blank lines).

I'd be consistent and use the same format everywhere. I don't think we are using the squared brackets in any other script. So, I'd simply use: echo "some message" everywhere. But feel free to check the log and pick the format that makes more sense, but use the same everywhere.

.travis.yml Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Show resolved Hide resolved
ci/setup_env.sh Show resolved Hide resolved
@datapythonista
Copy link
Member

and yes, please delete the replaced files in this same PR.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Check azure, your script is failing.

ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
ci/setup_env.sh Outdated Show resolved Hide resolved
@saurav2608
Copy link
Author

@datapythonista - Azure is failing on conda update specifically - conda update -q conda. I was reading here that conda now suggests conda update -n base conda. I can change the script to reflect this. However, why are we even doing this? We are downloading and installing the latest miniconda build. Why would we need to update conda just after that.

Another question is on line 32 of setup_env.sh. Why is the rm -rf is needed in the current setup. I would think that the VM running the tests is spun fresh everytime we start the test.

@datapythonista
Copy link
Member

Yes, please update the conda update statement to the new syntax. Anaconda can release a new conda version, but not create a new installable immediately, that's why it's common to update conda itself after installation. It's similar to when you install Linux and immediately run dnf update or equivalent. The iso is older than the versions of the latest packages.

I'm not sure about the rm -rf. I guess travis can reuse the containers or something. If the environment is clean for every build, it surely doesn't make sense. But please leave it for now, I'm sure it was added for a reason.

@saurav2608
Copy link
Author

@datapythonista / @jreback - any thoughts on this PR?

@jreback
Copy link
Contributor

jreback commented Jan 28, 2019

this needs to wait until after 0.24.1 is out at a minimum

@jreback
Copy link
Contributor

jreback commented Feb 16, 2019

can you merge master and i'll loo again here

@jreback jreback added this to the 0.25.0 milestone Feb 16, 2019
@saurav2608
Copy link
Author

@jreback - green.

@jreback
Copy link
Contributor

jreback commented Feb 19, 2019

@saurav2608 pls confirm for each build that we are running the same number of tests both before and after. This change could easy miss testing things and we would not easily know. (take the build on master right before you merged). check on both travis and azure; link the builds here.

@saurav2608
Copy link
Author

@jreback : I compared this build to one just before. Details -

PR platform No of Tests Link
#24632 Azure 336,141 https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=8496&view=ms.vss-test-web.build-test-results-tab
#25209 Azure 336,141 https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=8495&view=ms.vss-test-web.build-test-results-tab

For Travis the order was a bit different. But I compared the this build with the one just before. And compared 4 builds

PR>build>Job platform No of Tests Link
#24632 > Build #52470 > Job #52470.1 Travis 43095 passed, 2814 skipped, 1008 xfailed, 20 xpassed, 58 warnings in 678.23 seconds https://travis-ci.org/pandas-dev/pandas/jobs/494931336
#25362 > Build #52468 > Job #52468.1 Travis 43095 passed, 2814 skipped, 1008 xfailed, 20 xpassed, 58 warnings in 659.78 seconds https://travis-ci.org/pandas-dev/pandas/jobs/494917125
#24632 > Build #52470 > Job #52470.2 Travis 45407 passed, 1325 skipped, 783 xfailed, 20 xpassed, 24 warnings in 1144.70 seconds https://travis-ci.org/pandas-dev/pandas/jobs/494931338
#25362 > Build #52468 > Job #52468.2 Travis 45407 passed, 1325 skipped, 783 xfailed, 20 xpassed, 24 warnings in 1093.64 seconds https://travis-ci.org/pandas-dev/pandas/jobs/494917126
#24632 > Build #52470 > Job #52470.3 Travis 45338 passed, 917 skipped, 1013 xfailed, 20 xpassed, 42 warnings in 880.59 seconds https://travis-ci.org/pandas-dev/pandas/jobs/494931340
#25362 > Build #52468 > Job #52468.3 Travis 45338 passed, 917 skipped, 1013 xfailed, 20 xpassed, 42 warnings in 833.46 seconds https://travis-ci.org/pandas-dev/pandas/jobs/494917127
#24632 > Build #52470 > Job #52470.4 Travis 45410 passed, 842 skipped, 1014 xfailed, 21 xpassed, 90 warnings in 1305.06 seconds https://travis-ci.org/pandas-dev/pandas/jobs/494931341
#25362 > Build #52468 > Job #52468.4 Travis 45410 passed, 842 skipped, 1014 xfailed, 21 xpassed, 90 warnings in 1243.80 seconds https://travis-ci.org/pandas-dev/pandas/jobs/494917128

Copy link
Contributor

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Didn't see this PR before, I'll close #25377 once this gets merged.

ci/setup_env.sh Show resolved Hide resolved
@jreback
Copy link
Contributor

jreback commented Feb 20, 2019

@saurav2608 ok this looks fine. I still am going to wait till we do 0.24.2, we will have to backport this as well I think. We have had CI brokeness in the past from even minor changes in the CI code.

@WillAyd
Copy link
Member

WillAyd commented Feb 27, 2019

Can you merge master on this?

@h-vetinari
Copy link
Contributor

What's the status of this PR? I constantly need to look up manually which version is in failing posix environments. If this is still taking longer, can we merge #25377 in the interim?

@h-vetinari
Copy link
Contributor

What's the status of this PR? I constantly need to look up manually which version is in failing posix environments. If this is still taking longer, can we merge #25377 in the interim?

@jreback
Copy link
Contributor

jreback commented Mar 24, 2019

this needs a rebase and the PY2 and numpy bump merged first.

@jreback
Copy link
Contributor

jreback commented Mar 30, 2019

@saurav2608 if you wan to rebase this, i think we can do this now.

@saurav2608
Copy link
Author

@saurav2608 if you wan to rebase this, i think we can do this now.

@jreback - green

@jreback jreback merged commit b974633 into pandas-dev:master Apr 5, 2019
@jreback
Copy link
Contributor

jreback commented Apr 5, 2019

thanks @jreback and thanks @datapythonista for guiding this along.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: Simplify and unify environment creation TST: Review if we are really testing with different locales
6 participants