-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
CI: Some Azure Pipelines cleanups #23111
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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. |
ci/azure-Linux-35.yaml
Outdated
@@ -0,0 +1,28 @@ | |||
name: pandas |
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.
Minor issue but should "Linux" be all lower case in the file name to be consistent with the windows yaml files?
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.
Thanks very much for taking a look at this @vtbassmatt
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'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.)
@datapythonista I will definitely take a look. Probably Monday at this point. |
Boo, build is failing on Linux and I don't quite understand why. |
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. |
@vtbassmatt can you rebase? |
Rebased. Hopefully I didn't break anything -- I still have a shadow pipeline definition set up, so I'll know shortly. |
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 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
azure-pipelines.yml
Outdated
# 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 |
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.
Is this commented out right now, or is being substituted?
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.
Whoops, I really screwed this rebase up. Will fix it tomorrow.
No worries, I think it's our fault for letting this sit unmerged for too
long :) I believe the new linux builds were added after you opened the PR.
…On Mon, Nov 5, 2018 at 4:14 PM Matt Cooper ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In azure-pipelines.yml
<#23111 (comment)>:
>
-# 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
Whoops, I really screwed this rebase up. Will fix it tomorrow.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23111 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIi8ai5vgiGr7_eXdPO-q_306YSyPks5usLg_gaJpZM4XZuip>
.
|
Let's see if that rebase was a little better... |
I'm sorry, I wasn't watching this closely enough and missed the build failure. |
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. |
is this PR still relevant? @datapythonista |
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? |
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. |
@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? 😋 |
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.
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?
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. |
I'm working on the benchmarks failures (on azure) and the travis failures right now in #24092. |
We think we've spotted the bug - a missing null check when the build errors without a message. We're preparing a hotfix now. |
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. |
@TomAugspurger if you can take a look, and merge if you're happy |
I probably won't have time to look today, so I'd say go ahead and merge if
you're comfortable with the changes.
…On Mon, Dec 10, 2018 at 6:16 PM Marc Garcia ***@***.***> wrote:
@TomAugspurger <https://github.com/TomAugspurger> if you can take a look,
and merge if you're happy
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23111 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIubAK06_mdVogi8L_o53Kr4l_MVHks5u3vlZgaJpZM4XZuip>
.
|
Double checked the changes, and the build, and everything looks all right. Merging. Thanks @vtbassmatt |
* 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)
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 usemktemp -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.