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

[SPARK-8725][PROJECT-INFRA] Test modules in topologically-sorted order in dev/run-tests #10885

Closed
wants to merge 7 commits into from

Conversation

JoshRosen
Copy link
Contributor

This patch improves our dev/run-tests script to test modules in a topologically-sorted order based on modules' dependencies. This will help to ensure that bugs in upstream projects are not misattributed to downstream projects because those projects' tests were the first ones to exhibit the failure

Topological sorting is also useful for shortening the feedback loop when testing pull requests: if I make a change in SQL then the SQL tests should run before MLlib, not after.

In addition, this patch also updates our test module definitions to split sql into catalyst, sql, and hive in order to allow more tests to be skipped when changing only hive/ files.

@JoshRosen JoshRosen changed the title [SPARK-8725] Test modules in topologically-sorted order in dev/run-tests [SPARK-8725][PROJECT-INFRA] Test modules in topologically-sorted order in dev/run-tests Jan 24, 2016
@@ -0,0 +1,85 @@
#######################################################################
# Implements a topological sort algorithm.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen, I didn't want to write my own sort so I just copied this one from the toposort library. Do I need to update the NOTICE file in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping again. @srowen @pwendell, can you comment on the NOTICE file considerations here?

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I missed this. It looks like it's Apache licensed and you have correctly preserved the header. https://bitbucket.org/ericvsmith/toposort/src/25b5894c4229cb888f77cf0c077c05e2464446ac/LICENSE.txt?fileviewer=file-view-default

You'll need to put the NOTICE contents in our NOTICE:
https://bitbucket.org/ericvsmith/toposort/src/25b5894c4229cb888f77cf0c077c05e2464446ac/NOTICE?fileviewer=file-view-default

That should be all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I just append the entire contents of the notice verbatim? Is there a shorthand way of doing this (e.g. just including just the copyright)? I got a bit confused looking at the existing NOTICE file and since it hasn't been updated in a while I didn't spot any smaller patches / diffs to model my change on.

Copy link
Member

Choose a reason for hiding this comment

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

Yes just append. I don't know if we've got a strong format going, except to try to precede the text from Foo with a line like "Foo" or "For Foo:". Anything sane should be OK.

Technically you reproduce all relevant parts of the NOTICE and only relevant parts. All is relevant here. Really, not sure the project needed this file but hey. Practically you could argue you can abbreviate it, but given it's short, I'd just copy it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, updated the NOTICE to append it verbatim.

@SparkQA
Copy link

SparkQA commented Jan 24, 2016

Test build #49943 has finished for PR 10885 at commit 76a446e.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 24, 2016

Test build #49944 has finished for PR 10885 at commit c122c55.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

Whoops:

========================================================================
Traceback (most recent call last):
  File "./python/run-tests.py", line 42, in <module>
    from sparktestsupport.modules import all_modules  # noqa
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/../dev/sparktestsupport/modules.py", line 112, in <module>
    "sql/test",
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/../dev/sparktestsupport/modules.py", line 74, in __init__
    dep.dependent_modules.add(self)
TypeError: unhashable type: 'Module'

@SparkQA
Copy link

SparkQA commented Jan 24, 2016

Test build #49955 has finished for PR 10885 at commit 00a817a.

  • This patch fails executing the dev/run-tests script.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 24, 2016

Test build #49956 has finished for PR 10885 at commit a17c3d0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

Unless anyone has objections, I'm planning to merge this today. If anyone complains about the loss of Python 2.6 support in this script, they can submit a PR to fix compatibility themselves (again, this is only relevant to developers of Spark / maintainers of its test infrastructure, not end users, and I expect that they're using non-ancient Python versions or can easily upgrade).

@SparkQA
Copy link

SparkQA commented Jan 26, 2016

Test build #50113 has finished for PR 10885 at commit 0bc9da0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

Going to merge this now; will address any problems in followups.

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.

3 participants