Build MacOS wheels in separate Travis jobs #1205
Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
|
Merged build finished. Test FAILed. |
|
Test FAILed. |
python/build-wheel-macos.sh
Outdated
| "3.4" | ||
| "3.5" | ||
| "3.6") | ||
| if [[ "$PYTHON" == "2.7" ]]; then |
There was a problem hiding this comment.
It would be nice to leave the default behavior of this script unchanged so that it builds all of the wheels by default.
Instead of reading environment variables, one option would be to call this script from Travis and pass in different command line arguments.
There was a problem hiding this comment.
This script is intended to be useful outside of Travis, where we can't assume that the $PYTHON environment variables is set.
There was a problem hiding this comment.
Oh, I see! Thanks for context; I'll fix up sometime in the next day.
There was a problem hiding this comment.
Agree that environment variables could be problematic, so I implemented a simple command line argument. But I forgot that Travis has a nice column that tells you what environment variables are set for the build job; with the command line argument only, I see four jobs that all look (superficially) the same.
So what we could do is check if an environment variable is set; if yes, check that it's a valid version of Python and build the wheel for just that version. We could even choose something besides $PYTHON if that might be overloaded.
Up to you. I could definitely see the long-term use for using a CLI for build scripts. I'll let the cli version keep testing on Travis.
There was a problem hiding this comment.
Or derp, we could keep the CLI argument and pass in a Travis environment variable. That may be the best option.
|
Merged build finished. Test FAILed. |
|
Test FAILed. |
|
Merged build finished. Test PASSed. |
|
Test PASSed. |
|
Merged build finished. Test PASSed. |
|
Test PASSed. |
|
@danielsuo, yeah passing in a Travis environment variable on the command line makes sense. The aggregate time for the runs seems substantially longer now compared with when they are all done together (presumably because of the brew installations..). |
|
Or perhaps not. I based that statement off of some earlier runs, but the most recent were reasonably quick. |
|
Merged build finished. Test FAILed. |
|
Test FAILed. |
|
Yeah, there's a ton of variation in how long runs take. I'm looking at some of the recent builds and honestly, I'm a little jealous. How is everyone passing the In any case, it sounds like this isn't as high-priority as I thought since my experience was pretty bad. It's also a piecemeal approach to a more general problem (more flexible and portable build spec). |
|
I think there are periods of time when the MAC_WHEELS build passes fairly consistently, and there are times when it fails consistently (perhaps due to the load on Travis or something like that). You're right that in general having non-flaky tests is super high priority, so we probably do need to do something like this. It just occurred to me that we should email Travis and see if we can get longer build times, so let me give that a try. |
|
That would be great if they’d do that!
Otherwise, he free version is 50 minutes per job (or thereabouts) and the
paid (kinda pricy if we want concurrent jobs) gets you 120.
…On Tue, Nov 14, 2017 at 1:47 AM Robert Nishihara ***@***.***> wrote:
I think there are periods of time when the MAC_WHEELS build passes fairly
consistently, and there are times when it fails consistently (perhaps due
to the load on Travis or something like that).
You're right that in general having non-flaky tests is super high
priority, so we probably do need to do something like this. It just
occurred to me that we should email Travis and see if we can get longer
build times, so let me give that a try.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1205 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACkV1tfg9LbRaEkqHiXZ88Hp3xxElkGgks5s2TeDgaJpZM4QZJtk>
.
|
|
Tangentially related: turns out we can set and get variables in We set a variable via |
|
Oh that's nice, that seems useful. |
|
@danielsuo I think it's probably safe to close this issue, what do you think? I haven't seen any MacOS jobs time out (on the main fork). I have see Travis jobs time out on my own fork, which makes sense because the job times were extended only for the main repository. |
|
Feel free to close! Two arguments in favor (for completeness):
1. It would be good for Travis to pass on contributor forks. Not
everyone will want to develop in a PR of the main branch (airing dirty
laundry and all that).
2. It seems like something that will come up anyway in the future
But again, feel free to close. If we were really addressing the above two
points, I'd handle differently (described in part earlier in the
discussion). After all, this was just a hack so things would stop failing.
…On Sun, Nov 26, 2017 at 6:13 PM, Robert Nishihara ***@***.***> wrote:
@danielsuo <https://github.com/danielsuo> I think it's probably safe to
close this issue, what do you think?
I haven't seen any MacOS jobs time out (on the main fork). I have see
Travis jobs time out on my own fork, which makes sense because the job
times were extended only for the main repository.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1205 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACkV1oc-Oy7CSzeUhN57LHZt6yMq8rngks5s6fChgaJpZM4QZJtk>
.
|
|
Thanks, those are both good points. I'd prefer to close it for now, but you're right and we will most likely need to revisit this. |
Python 2.7, 3.4, 3.5, and 3.6 wheels all build in their own Travis jobs now.
Notes:
.travis.ymlin Speed up Mac OS Travis builds. #1110 as a possible solution.$TRAVIS_COMMIT/.whldoesn't get overwritten in some bad way