nchammas commented on code in PR #58555:
URL: https://github.com/apache/spark/pull/58555#discussion_r4027055359
##########
dev/sparktestsupport/modules.py:
##########
@@ -36,32 +36,140 @@
# - `.gitignore`-style patterns would be ideal but don't have support in the
# standard library.
ignored_file_patterns = (
- ".asf.yaml",
- ".gitignore",
+ # Contributor docs.
"AGENTS.md",
+ "CLAUDE.md",
"CONTRIBUTING.md",
+ "PULL_REQUEST_TEMPLATE",
"README.md",
+ "SECURITY.md",
+ # License and NOTICE files, including bundled third-party licenses.
"/LICENSE-binary",
+ "/LICENSE",
+ "/licenses-binary/",
+ "/licenses/",
"/NOTICE-binary",
- "/scalastyle-config.xml",
- "/SECURITY.md",
+ "/NOTICE",
+ # Git and ASF metadata.
+ ".asf.yaml",
+ ".gitattributes",
+ ".gitignore",
+ # Build and test workflow wrappers or schedulers.
+ "/.github/workflows/benchmark.yml",
+ "/.github/workflows/branch*_scheduler.yml",
+ "/.github/workflows/build_codegen_jdk.yml",
+ "/.github/workflows/build_coverage.yml",
+ "/.github/workflows/build_java*.yml",
+ "/.github/workflows/build_main.yml",
+ "/.github/workflows/build_maven*.yml",
+ "/.github/workflows/build_non_ansi.yml",
+ "/.github/workflows/build_python_3*.yml",
+ "/.github/workflows/build_python_connect*.yml",
+ "/.github/workflows/build_python_minimum.yml",
+ "/.github/workflows/build_python_pypy3.10.yml",
+ "/.github/workflows/build_rockdb_as_ui_backend.yml",
+ "/.github/workflows/build_scala213.yml",
+ "/.github/workflows/build_sparkr_window.yml",
+ "/.github/workflows/build_uds.yml",
+ "/.github/workflows/maven_test.yml",
+ "/.github/workflows/python_hosted_runner_test.yml",
+ # GitHub Pages.
+ ".nojekyll",
+ "/.github/workflows/pages.yml",
+ # Release and publishing.
+ "/.github/workflows/publish_snapshot.yml",
+ "/.github/workflows/release.yml",
+ # GitHub housekeeping and repo automation.
+ "/.github/workflows/build_infra_images_cache.yml",
+ "/.github/workflows/images/",
+ "/.github/workflows/notify_test_workflow.yml",
+ "/.github/workflows/stale.yml",
+ "/.github/workflows/test_report.yml",
+ "/.github/workflows/update_build_status.yml",
+ # Linters, formatters, and their configs. These are typically triggered by
+ # the `precondition` job regardless of modifications to these files.
+ # precondition job:
https://github.com/apache/spark/blob/065397779ce409996abed999e1d6b46ee98a6c34/.github/workflows/build_and_test.yml#L106-L199
+ # lint job:
https://github.com/apache/spark/blob/065397779ce409996abed999e1d6b46ee98a6c34/.github/workflows/build_and_test.yml#L932-L935
+ # buf job:
https://github.com/apache/spark/blob/065397779ce409996abed999e1d6b46ee98a6c34/.github/workflows/build_and_test.yml#L895-L898
+ ".pre-commit-config.yaml",
+ "/dev/.rat-excludes",
+ "/dev/.scalafmt.conf",
+ "/dev/check_pyspark_custom_errors.py",
+ "/dev/check-license",
+ "/dev/check-protos.py",
"/dev/checkstyle-suppressions.xml",
"/dev/checkstyle.xml",
+ "/dev/connect-jvm-client-mima-check",
+ "/dev/eslint.js",
+ "/dev/java-file-header",
+ "/dev/lint-java",
+ "/dev/lint-js",
+ "/dev/lint-python",
+ "/dev/lint-r*",
+ "/dev/lint-scala",
+ "/dev/mima",
+ "/dev/package-lock.json",
+ "/dev/package.json",
+ "/dev/reformat-python",
+ "/dev/sbt-checkstyle",
+ "/dev/scalafmt",
+ "/dev/scalastyle",
+ "/dev/spark-test-image/lint/",
+ "/dev/structured_logging_style.py",
+ "/scalastyle-config.xml",
+ # Maintainer tools.
+ "/dev/*gen-protos.sh",
+ "/dev/change-scala-version.sh",
"/dev/create_jira_and_branch.py",
"/dev/create_spark_jira.py",
"/dev/create-release/",
- "/dev/lint-python",
- "/dev/lint-scala",
+ "/dev/free_disk_space_container",
+ "/dev/free_disk_space",
+ "/dev/generate_srs_registry.py",
"/dev/make-distribution.sh",
"/dev/merge_spark_pr.py",
+ "/dev/next_version_candidates.py",
"/dev/pr_merge_status.py",
- "/dev/reformat-python",
- "/dev/requirements.txt",
+ "/dev/protobuf-breaking-changes-check.sh",
+ "/dev/py-cleanup",
+ "/dev/requirements.txt", # legacy, replaced in CI by pyproject.toml
+ "/dev/spark_jira_utils.py",
"/dev/spark_merge_footer.py",
- "/dev/spark-test-image/lint/Dockerfile",
- "/dev/structured_logging_style.py",
- "/ui-test/package-lock.json",
- "/ui-test/package.json",
+ "/dev/spark-test-image/connect-gen-protos/",
+ "/dev/spark-test-image/python-*/Dockerfile",
+ "/sql/create-docs.sh",
+ # Cluster admin scripts, CLI wrappers, and other user executables.
+ # These scripts are currently not covered by any existing test suite
+ # invoked from here.
+ "/bin/beeline*",
+ "/bin/docker-image-tool.sh",
+ "/bin/run-example*",
+ "/bin/spark-connect-shell",
+ "/bin/spark-pipelines",
+ "/bin/sparkR*",
+ "/sbin/decommission-worker.sh",
+ "/sbin/spark-daemons.sh",
+ "/sbin/start-all.sh",
+ "/sbin/start-connect-server.sh",
Review Comment:
I think we have different visions for `sparktestsupport` and it would be
good to clarify and resolve those differences because I am planning a lot of
work in this area.
> The files listed in ignore list should be obvious that they are not used
by pyspark.
This is perhaps already clear, but just to be explicit: The test selection
driven by `sparktestsupport` is not specific to PySpark. It covers all of
Spark. `sparktestsupport` selects what tests to run based on what repo paths
are modified. Those tests could be PySpark tests; they could be Scala tests;
down the line we could (and should) add other types of tests that are currently
captured elsewhere, like linters and shell script tests.
> It's definitely possible that in the future scripts like
stop-connect-server.sh is used by pyspark, and it would be very confusing to
people who do not fully understand how this system works, when they realize
their CI does not run. Or even worse, they probably do not even realize that.
I agree, but the correct thing to do is to add the sbin scripts to a module
in `sparktestsupport` that tests them, rather than allow the test selection to
[fall through to the `root` module][1] and run ~30h of irrelevant tests in the
hopes that it will somehow test what we want.
[1]:
https://github.com/apache/spark/blob/65ceabb64bddd019b851f6325271eee178c71ef1/dev/sparktestsupport/utils.py#L57-L58
This is the effort I am undertaking in SPARK-59180. This effort will span
many PRs, including this one. My goal is to improve the precision of CI test
selection so we can reduce waste and improve response times. I ultimately want
every repo path to either map to at least one test module or be ignored. I am
planning at least one new test module specifically for the sbin scripts; I will
tag you on those PRs if you like.
I'll remove the sbin scripts from the ignore list here since I would be
removing them anyway as part of SPARK-59566. But I wanted to explain this
background since my reason for doing so is different than yours.
Does the big picture of SPARK-59180 make sense to you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
