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

Support buildkite CICD and restructure smoke tests #4396

Merged
merged 73 commits into from
Dec 13, 2024

Conversation

zpoint
Copy link
Collaborator

@zpoint zpoint commented Nov 22, 2024

The PR is based on #4386. Please review and merge that PR first, then review and merge this one.

  • Restructure test_smoke.py into multiple files within this directory.
tests/smoke_tests
├── __init__.py
├── test_basic.py
├── test_cluster_job.py
├── test_images.py
├── test_managed_job.py
├── test_mount_and_storage.py
├── test_region_and_zone.py
├── test_required_before_merge.py
├── test_sky_serve.py
└── smoke_tests_utils.py
  • Add a directory .buildkite to contain all the buildkite-related files.
    • Test cases in test_required_before_merge.py will be triggered only by valid users' comment on a PR.
      • Support format: /pre-merge-test aws / /pre-merge-test aws,azure / /pre-merge-test(default aws)
    • Other test cases in different files will be run before release and triggered manually.
.buildkite
├── generate_pipeline.py  -- script to generate following yaml automatically
├── pipeline_smoke_tests_pre_merge.yaml -- pre merge smoke tests, only the minimal requried, generated from tests/smoke_tests/test_required_before_merge.py
└── pipeline_smoke_tests_release.yaml -- release smoke tests, all smoke tests included, generated from tests/smoke_tests/test_*.py
  • Support the LOG_TO_STDOUT environment variable when executing pytest, so that the output is visible directly on the CI/CD terminal page

  • Support the SKYPILOT_SUPPRESS_SENSITIVE_LOG environment variable when executing pytest to suppress logs from provision.py, ensuring sensitive information is not exposed during pre-merge tests.

  • Made a minor change to .pre-commit-config.yaml after discovering that pre-commit wasn't working for files under .buildkite.

Test

Don't have permission for this repo, currently tested under my personal repo

image

Per PR merged CI

  • I configured the pre-PR Buildkite pipeline on my personal forked repository.
  • You can see an example PR on my personal repository (GitHub integration is visible in that PR).

Scheduled CI

A build attempt link

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

@zpoint zpoint requested a review from Michaelvll December 2, 2024 07:55
@zpoint zpoint force-pushed the dev/zeping/restructure_smoke_tests branch 4 times, most recently from b3a5520 to cd64c4c Compare December 4, 2024 10:08
@Michaelvll Michaelvll requested a review from cg505 December 6, 2024 20:16
Copy link
Collaborator

@cg505 cg505 left a comment

Choose a reason for hiding this comment

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

Thank you for this change which will be super helpful, @zpoint! I left comments primarly on generate_pipeline.py.

I did not review the smoke_tests/ directory, as I understand these were mostly juts copied from the old test_smoke.py. Were there any significant changes in the tests?

.buildkite/generate_pipeline.py Outdated Show resolved Hide resolved
.buildkite/generate_pipeline.py Outdated Show resolved Hide resolved
.buildkite/generate_pipeline.py Outdated Show resolved Hide resolved
.buildkite/generate_pipeline.py Outdated Show resolved Hide resolved
.buildkite/generate_pipeline.py Outdated Show resolved Hide resolved
.buildkite/generate_pipeline.py Outdated Show resolved Hide resolved
.buildkite/generate_pipeline.py Show resolved Hide resolved
.buildkite/generate_pipeline.py Show resolved Hide resolved
.buildkite/generate_pipeline.py Show resolved Hide resolved
@zpoint
Copy link
Collaborator Author

zpoint commented Dec 10, 2024

I did not review the smoke_tests/ directory, as I understand these were mostly juts copied from the old test_smoke.py. Were there any significant changes in the tests?

@cg505 Thanks for your review! Yes, it's mostly copied, with some private names starting with _ changed to public names. I will restore and copy back the test_smoke.py file for review, and after the review, we can delete this file. What do you think?

@zpoint zpoint requested a review from cg505 December 10, 2024 04:30
Copy link
Collaborator

@cg505 cg505 left a comment

Choose a reason for hiding this comment

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

Thanks, looks pretty much good to me. Left a couple more comments. Two additional things:

  1. I think ideally we will want to move toward using the pytest marks as intended, and split things up/fix the tests to work better in parallel. Then we can avoid this hacky ast parsing method. For now, this is fine.
  2. Can we look into enforcing lint (pylint, mypy, yapf, etc) on tests/ dir?

Neither of these should be in this PR, just things for the future.

tests/test_yamls/minimal_test_required_before_merge.yaml Outdated Show resolved Hide resolved
.buildkite/generate_pipeline.py Outdated Show resolved Hide resolved
tests/backward_compatibility_tests.sh Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
@zpoint
Copy link
Collaborator Author

zpoint commented Dec 11, 2024

@cg505 Thanks, the comment has been resolved.

Yes, I agree:

  1. We need more time to refactor and reorganize the pytest.
  2. Currently, the tests/ folder does not follow our linting rules, and we need additional time to fix the linting issues.

Both tasks will require time, we can address them in a future PR.

@zpoint zpoint requested a review from cg505 December 11, 2024 10:32
Copy link
Collaborator

@cg505 cg505 left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes! Looks good to me now!

@cg505
Copy link
Collaborator

cg505 commented Dec 12, 2024

Probably ok to merge this to avoid any more smoke test conflicts with master, and we can figure out the remaining buildkite agent/deployment questions after

@zpoint zpoint merged commit e4ea276 into skypilot-org:master Dec 13, 2024
19 checks passed
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.

2 participants