gaogaotiantian commented on PR #58274: URL: https://github.com/apache/spark/pull/58274#issuecomment-5419344795
Thanks for the review @dongjoon-hyun and @Yicong-Huang. First of all, it was my oversight that the coverage for pyarrow upstream was reduced - that was not intentional. I fully agree that it should be executed on all platforms + python versions. The implementation is worth discussing. I agree `pyspark-scheduled` is not the best name, but we have 4 kinds of tests that we need to include in this mechanism: 1. `test_install_spark` - installation 2. `test_import_spark` - make sure `import pyspark` does not include unnecessary 3rd party libraries 3. All upstream tests to detect if `pyarrow` upgrade changes the current result 4. packaging test which is *not* a python test. It lives in a `sh` file and is currently triggered by detecting whether the module includes `pyspark-install`. We can't easily put it into a pyspark test (maybe we can but it requires serious refactoring). So this does *not* belong to any module. Is the name `pyspark-periodic` better? Still summarizes the common features of these tests and does not indicate these only runs on scheduled CIs. One thing I would propose in this PR is to make the current `pyspark-install` tests run on every scheduled Python CI too. Packaging test actually can be impacted by the python version and architecture. We used to have a very slow install test because we are pointing to an archived version. The current install test is pretty fast so we can afford to run it in scheduled CI. The good thing about running `all these tests on every scheduled CI, is that we don't have to have 2 separate mechanisms for triggering them. We only need a single mechanism to figure out if this is a scheduled CI, or a main commit, which should be pretty simple - just need to check the repo and the event. **Proposal:** 1. Use `pyspark-periodic` to replace `pyspark-install`. 2. Make `pyspark-periodic` run on all pre-merge CIs and all scheduled CIs, not just coverage. We are increasing some python version and architecture coverage for `test_install_spark` and `test_import_spark`, not reducing. The cost should not be too large. WDYT? -- 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]
