Skip to content

Conversation

@dmvieira
Copy link

@dmvieira dmvieira commented Aug 7, 2017

What changes were proposed in this pull request?

I was doing PR #18802 and tests always fail.

Here I'm fixing Jenkins tests that were failing with python 2.6. Here there are some backports for python 2.6

How was this patch tested?

Tests passing at Jenkins

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@vanzin
Copy link
Contributor

vanzin commented Aug 7, 2017

This doesn't look right. There are way too many changes, including the ones from the PR you mention which should not be in this PR.

Can you include just the test fixes, and if they apply, the original bug numbers too?

I'm a little confused about the need for this, since Jenkins tests are green:
https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/

So more context will help.

@srowen
Copy link
Member

srowen commented Aug 7, 2017

Python 2.6 isn't supported either, so what's this trying to do?

@vanzin
Copy link
Contributor

vanzin commented Aug 7, 2017

It's supported in 2.1, right?

@srowen
Copy link
Member

srowen commented Aug 7, 2017

Ah right this is open vs 2.1. That much makes sense, disregard me.

@dmvieira dmvieira force-pushed the fix-python-2.6-tests branch from 81dc26b to 01e7ea9 Compare August 7, 2017 19:34
@dmvieira
Copy link
Author

dmvieira commented Aug 7, 2017

removed other PR related code

dev/run-tests.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Looks we can keep , sort=True)?

@vanzin
Copy link
Contributor

vanzin commented Aug 7, 2017

Is this a backport of another fix? If so you the PR title / summary should reflect that.

If not, you need to open a bug and provide a better explanation in the PR title / summary.

See: http://spark.apache.org/contributing.html

@HyukjinKwon
Copy link
Member

@vanzin, I remember this issue was fixed roughly 5 days ago, which might be the cause of test failures in 18802. Would it be worth trying to run the tests in that PR to check this if I understood correctly?

@vanzin
Copy link
Contributor

vanzin commented Aug 7, 2017

Tests are already running on the other PR, so we can see whether things still fail.

@dmvieira dmvieira force-pushed the fix-python-2.6-tests branch from 01e7ea9 to 5a6af72 Compare August 7, 2017 20:01
@vanzin
Copy link
Contributor

vanzin commented Aug 7, 2017

@dmvieira the other PR passed tests, doesn't seem like this is necessary.

@dmvieira
Copy link
Author

dmvieira commented Aug 8, 2017

It doesn't make sense... If you see all failed builds they're failing because doesn't support python 2.6... I think your CI is running python 2.6 in some machines and python 2.7 or higher in others. Are you sure that want to close this PR?

@HyukjinKwon
Copy link
Member

I think we are supposed to run the tests in Python 2.7 but it looks mistakenly used Python 2.6 for a while, which I guess caused the test failures, see SPARK-21573. I think it should be fine to leaving as is and closing this one for now.

@felixcheung
Copy link
Member

it was pointed out this PR is for branch-2.1, which should still support python 2.6, right?
@dmvieira you should probably put [BRANCH-2.1] or [BACKPORT] to the PR title. Please open a JIRA and list it as suggested http://spark.apache.org/contributing.html above

@HyukjinKwon
Copy link
Member

@felixcheung, Yes, that's right. I would support this if it was a simple backport and the Jenkins build still fails.

However, now it was fixed and the actual gain looks quite small - if I understood correctly, this change is useful only when someone manually runs tests by Python 2.6 in branch-2.1, whereas the changes look introducing untested workarounds although I know most of the current changes are correct as I already tried almost identical changes in my local before.

Another potential problem might be, backporting other changes would be harder. As an extreme example, there was actually a mistake during backporting some changes into this script before and I corrected this - 4a67541

I personally would like to avoid this change unless there are other good reasons to make this change alone in this branch ..

@srowen
Copy link
Member

srowen commented Aug 8, 2017

I agree, don't think this is worth changing. Python 2.6 was still intended to work but deprecated in 2.1, so not sure we need more support for testing 2.6. I am not sure there would be another 2.1.x release anyway.

@dmvieira
Copy link
Author

dmvieira commented Aug 8, 2017

Hey guys, I just opened this PR because I spent a lot of time trying to fix Jenkins tests in my last PR when the error was in test script with python 2.6... I can close it, but I know that more guys will spend more time trying to fix it again. Ok @felixcheung

@dmvieira dmvieira changed the title Fixing python 2.6 tests for jenkings [BRANCH-2.1][BACKPORT] Fixing python 2.6 tests for jenkings Aug 8, 2017
@felixcheung
Copy link
Member

felixcheung commented Aug 8, 2017 via email

@dmvieira
Copy link
Author

dmvieira commented Aug 9, 2017

Closing here... Thank you @felixcheung , @srowen , @vanzin and @HyukjinKwon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants