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: Some Azure Pipelines cleanups #23111

Merged
merged 5 commits into from Dec 11, 2018
Merged

CI: Some Azure Pipelines cleanups #23111

merged 5 commits into from Dec 11, 2018

Conversation

vtbassmatt
Copy link
Contributor

Hey all, I'm a PM on Azure Pipelines. I noticed you had some comments in your azure-pipelines.yml regarding conditional steps and possibly adding a Linux flavor. I've taken the liberty of addressing those issues.

One thing I didn't completely finish: getting Linux to publish test results. The problem is that there are directories the publish task can't read already sitting in /tmp. I think the right solution is to use mktemp -d and then use that directory in the test scripts. But that felt like a pretty invasive change that you'd want to discuss before I just powered through and did it :)

Also, the contents of azure-Linux-35.yaml are identical to macOS right now, because I wasn't sure what needed to differ between those environments.

@codecov
Copy link

codecov bot commented Oct 12, 2018

Codecov Report

Merging #23111 into master will increase coverage by 49.43%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #23111       +/-   ##
===========================================
+ Coverage   42.52%   91.95%   +49.43%     
===========================================
  Files         161      161               
  Lines       51697    51831      +134     
===========================================
+ Hits        21982    47660    +25678     
+ Misses      29715     4171    -25544
Flag Coverage Δ
#multiple 90.61% <ø> (?)
#single 42.88% <ø> (+0.36%) ⬆️
Impacted Files Coverage Δ
pandas/core/computation/pytables.py 92.37% <0%> (+0.3%) ⬆️
pandas/io/pytables.py 92.3% <0%> (+0.92%) ⬆️
pandas/util/_test_decorators.py 93.24% <0%> (+4.05%) ⬆️
pandas/compat/__init__.py 58.36% <0%> (+8.17%) ⬆️
pandas/core/config_init.py 99.24% <0%> (+9.84%) ⬆️
pandas/core/reshape/util.py 100% <0%> (+11.53%) ⬆️
pandas/compat/numpy/__init__.py 92.85% <0%> (+14.28%) ⬆️
pandas/core/computation/common.py 85.71% <0%> (+14.28%) ⬆️
pandas/core/api.py 100% <0%> (+14.81%) ⬆️
pandas/core/indexes/api.py 99% <0%> (+14.85%) ⬆️
... and 118 more

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 d7e96d8...87fb3f9. Read the comment docs.

@datapythonista datapythonista added Testing pandas testing functions or related to the test suite CI Continuous Integration labels Oct 12, 2018
@datapythonista
Copy link
Member

Thanks for the clean up @vtbassmatt. I don't know much about the Windows part, but lgtm. @TomAugspurger ?

@vtbassmatt unrelated to this PR, I'm working on moving the linting from travis to azure-pipelines in #22854. If you have feedback about it, that would be great.

@@ -0,0 +1,28 @@
name: pandas
Copy link
Member

Choose a reason for hiding this comment

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

Minor issue but should "Linux" be all lower case in the file name to be consistent with the windows yaml files?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks very much for taking a look at this @vtbassmatt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're welcome! I was cheating a bit by using the parameters.name as the ENV_FILE file name in the Linux/macOS case. Since it's cased as a display name, I matched that. Maybe I should capitalize Windows to match? Or I can think of another solution. (We don't have string.ToLower in our expression language.)

@vtbassmatt
Copy link
Contributor Author

@datapythonista I will definitely take a look. Probably Monday at this point.

@vtbassmatt
Copy link
Contributor Author

vtbassmatt commented Oct 16, 2018

Boo, build is failing on Linux and I don't quite understand why.

@TomAugspurger
Copy link
Contributor

I haven't looked too deeply, but the logs in https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=1216&view=logs reference numpy 1.10. We recently bumped our minimum to numpy 1.12.

@datapythonista datapythonista changed the title Some Azure Pipelines cleanups CI: Some Azure Pipelines cleanups Nov 4, 2018
@datapythonista
Copy link
Member

@vtbassmatt can you rebase?

@vtbassmatt
Copy link
Contributor Author

Rebased. Hopefully I didn't break anything -- I still have a shadow pipeline definition set up, so I'll know shortly.

@vtbassmatt
Copy link
Contributor Author

Mine looks clean.

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

I think there are some issues here. I believe we lost the following builds (which were added after you started this PR @vtbassmatt)

  • azure-27-compat
  • azure-36-locale_slow
  • azure-37-locale

I don't see a linux build in https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=2458&view=logs

So, could we add an ENV_FILE to the parameters passed to the macos-linux file, and then use those parameters in azure-pipelines.yml to have the following builds

  • macOS, ci/azure-macos-35.yaml
  • linux, azure-27-compat
  • linux, azure-36-locale_slow
  • linux, azure-37-locale

# Windows Python 2.7 needs VC 9.0 installed, and not sure
# how to make that a conditional task, so for now these are
# separate templates as well
# - template: ci/azure/macos-linux.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this commented out right now, or is being substituted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, I really screwed this rebase up. Will fix it tomorrow.

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Nov 5, 2018 via email

@vtbassmatt
Copy link
Contributor Author

Let's see if that rebase was a little better...

@vtbassmatt
Copy link
Contributor Author

I'm sorry, I wasn't watching this closely enough and missed the build failure.

@vtbassmatt
Copy link
Contributor Author

It looks like the Mac agent freaked out on that last run. It was working in the prior run and I didn't change anything, plus it worked on my private copy of the pipeline. I think this is back in working order.

@jreback
Copy link
Contributor

jreback commented Dec 3, 2018

is this PR still relevant? @datapythonista

@datapythonista
Copy link
Member

yes, that's something we'd like to have, as among other things it avoids repetitive code in the CI config.

@vtbassmatt can you merge master and fix the conflicts? And as I think you're addressing different things here, do you think it makes sense to split this PR, so we can merge the parts that are ready?

@vtbassmatt
Copy link
Contributor Author

I'll take a look tomorrow. I had already resolved a similar rebase before, and it took a bit of finesse to make sure I didn't break anything.

@vtbassmatt
Copy link
Contributor Author

@datapythonista pending the outcome of the Azure build, I think this is ready to merge. Can we please merge it soon so I don't have to rebase again? 😋

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.

Thanks @vtbassmatt, nice job, great to have this simplified.

lgtm, but I'm a bit lost reviewing the Windows part, @TomAugspurger can you take a look please?

@datapythonista
Copy link
Member

Btw @vtbassmatt I think the problem we've got with Azure not reporting the end of the build to GitHub happens in all the PRs now, including this one. Not sure if it happens to other projects too, but probably worth reporting it again.

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Dec 4, 2018

I'm working on the benchmarks failures (on azure) and the travis failures right now in #24092.

@vtbassmatt
Copy link
Contributor Author

vtbassmatt commented Dec 5, 2018

We think we've spotted the bug - a missing null check when the build errors without a message. We're preparing a hotfix now.

@datapythonista
Copy link
Member

That's great, thanks @vtbassmatt, and thanks for keeping us updated.

@TomAugspurger, now that #24092 has been merged, if you can take a look at this PR, I think it should be ready to be merged too.

@jreback jreback added this to the 0.24.0 milestone Dec 5, 2018
@datapythonista datapythonista self-assigned this Dec 11, 2018
@datapythonista
Copy link
Member

@TomAugspurger if you can take a look, and merge if you're happy

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Dec 11, 2018 via email

@datapythonista
Copy link
Member

Double checked the changes, and the build, and everything looks all right. Merging.

Thanks @vtbassmatt

@datapythonista datapythonista merged commit 9ea8176 into pandas-dev:master Dec 11, 2018
thoo added a commit to thoo/pandas that referenced this pull request Dec 12, 2018
* upstream/master:
  pct change bug issue 21200 (pandas-dev#21235)
  DOC: Fix summaries not ending with a period (pandas-dev#24190)
  DOC: Add back currentmodule to api.rst (pandas-dev#24232)
  DOC: Fixed implicit imports for whatsnew (v >= version 20.0) (pandas-dev#24199)
  remove enum import for PY2 compat, xref pandas-dev#22802 (pandas-dev#24170)
  CI: Simplify Azure Pipelines configuration files (pandas-dev#23111)
  REF/TST: Add more pytest idiom to indexing/multiindex/test_getitem.py (pandas-dev#24053)
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants