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

Add a pre commit config to help format before pushing #4258

Merged
merged 10 commits into from
Nov 27, 2024

Conversation

zpoint
Copy link
Collaborator

@zpoint zpoint commented Nov 4, 2024

Step 1 install pre-commit

pip install pre-commit
# in the project root of skypilot repo
pre-commit install
# Make sure the .pre-commit-config.yaml file is in your project root, as demonstrated in this PR.

Step 2, now just run git commit. The command will use the pre-commit hook, it will behave exactly the same as format.sh

image

Refer to this comment if you're using cursor/vscode and encounter env problem

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

@Michaelvll
Copy link
Collaborator

Thanks @zpoint! I was testing it in one of my PR #4264 and trying to use the commit in vscode/cursor's GUI, but it seems the pre-commit hook was not triggered. Do you know how to get it work?
image

@zpoint
Copy link
Collaborator Author

zpoint commented Nov 5, 2024

Did u put the .pre-commit-config.yaml in your project root directory?

Open a terminal, cd to your project root, try command pre-commit, it should output like my screenshots, if not check your $PATH variable, the pre-commit installed with the pip needs to be in your $PATH

image

@zpoint zpoint mentioned this pull request Nov 6, 2024
5 tasks
@zpoint
Copy link
Collaborator Author

zpoint commented Nov 9, 2024

Could you take a look when you have time? Thanks @Michaelvll

@Michaelvll
Copy link
Collaborator

Fixing sky/jobs/controller.py

fix end of files.........................................................Passed
check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
black................................................(no files to check)Skipped
isort....................................................................Passed
isort................................................(no files to check)Skipped
yapf.....................................................................Failed
- hook id: yapf
- files were modified by this hook
pylint...................................................................Passed
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

Executable `mypy` not found

Thanks @zpoint! Just realized that I did not install pre-commit in my python env. After installing it, it shows the above error for failing to find mypy in VSCode/Cursor's git integration, although I do have mypy installed in my separate python env for skypilot. Is there away for the VSCode git integration to correctly pick up the env?

@zpoint
Copy link
Collaborator Author

zpoint commented Nov 12, 2024

@Michaelvll

VSCode seems not to provide a way to configure the environment when you click the commit on the GUI menu.
This issue is still opened for VSCode after 4 years.

Workaround1:

The workaround is to start VSCode from the command line after you activate your environment; in this way, VSCode will carry all the environment variables from your command line.

If you are not using zsh, refer to: how-to-open-cursor-from-terminal

# add this to the tail of your ~/.zshrc
function cursor {
  open -a "/Applications/Cursor.app" "$@"
}

# activate my env
conda activate sky
# now, cursor will start with current env
cursor .

# Or you can activate your env inside the cursor function, then just type cursor will do

workaround2:

Install mypy globally in the global Python environment, which is not elegant.

@zpoint zpoint requested a review from Michaelvll November 25, 2024 03:35
Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks @zpoint! I am good with having this PR in. Could we add a TODO comment in the config yaml to keep it sync with the format.sh. We should add the instruction in the developer docs and mark it optional: https://docs2.skypilot.co/en/latest/developers/CONTRIBUTING.html

@zpoint
Copy link
Collaborator Author

zpoint commented Nov 26, 2024

Updated the document. Could you help take another look before merging? @Michaelvll

@zpoint
Copy link
Collaborator Author

zpoint commented Nov 27, 2024

I double-checked and fixed this configuration; it should now match the format.sh script. @Michaelvll

@zpoint zpoint added this pull request to the merge queue Nov 27, 2024
Merged via the queue into skypilot-org:master with commit c3223a4 Nov 27, 2024
20 checks passed
@zpoint zpoint deleted the dev/zeping/pre_commit branch November 27, 2024 06:00
@zpoint zpoint mentioned this pull request Nov 27, 2024
5 tasks
zpoint added a commit to zpoint/skypilot that referenced this pull request Nov 27, 2024
@zpoint
Copy link
Collaborator Author

zpoint commented Nov 27, 2024

Cusor

This won't be a problem now. I have configured the pre-commit file to use its own environment instead of the local one. As long as the pre-commit command is in your PATH and Cursor can find it, everything will work fine.

Michaelvll added a commit that referenced this pull request Dec 9, 2024
* [perf] optimizations for sky jobs launch (#4341)

* cache AWS get_user_identities

With SSO enabled (and maybe without?) this takes about a second. We already use
an lru_cache for Azure, do the same here.

* skip optimization for sky jobs launch --yes

The only reason we call optimize for jobs_launch is to give a preview of the
resources we expect to use, and give the user an opportunity to back out if it's
not what they expect. If you use --yes or -y, you don't have a chance to back
out and you're probably running from a script, where you don't care.
Optimization can take ~2 seconds, so just skip it.

* update logging

* address PR comments

* [ux] cache cluster status of autostop or spot clusters for 2s (#4332)

* add status_updated_at to DB

* don't refresh autostop/spot cluster if it's recently been refreshed

* update locking mechanism for status check to early exit

* address PR comments

* add warning about cluster status lock timeout

* [k8s] fix managed job issue on k8s (#4357)

Signed-off-by: nkwangleiGIT <[email protected]>

* [Core] Add `NO_UPLOAD` for `remote_identity` (#4307)

* Add skip flag to remote_identity

* Rename to NO_UPLOAD

* Fixes

* lint

* comments

* Add comments

* lint

* Add Lambda's GH200 instance type (#4377)

Add GH200 instance type

* [FluidStack] Fix provisioning and add new gpu types (#4359)

[FluidStack] Fix provisioning and add new gpu types

    * Add new `provisioning` status to fix failed deployments

    * Add H100 SXM5 GPU mapping

* [ux] display human-readable name for controller (#4376)

* [k8s] Handle apt update log not existing (#4381)

do not panic if file does not exist, it may be written soon

* Support event based smoke test instead of sleep time based to reduce flaky test and faster test (#4284)

* event based smoke test

* more event based smoke test

* more test cases

* more test cases with managed jobs

* bug fix

* bump up seconds

* merge master and resolve conflict

* restore sleep for fail test case

* [UX] user-friendly message shown if Kubernetes is not enabled. (#4336)

try except

* [Jobs] Disable deduplication for logs (#4388)

Disable dedup

* [OCI] set zone in the ProvisionRecord (#4383)

* fix: Add zone to the ProvisionRecord

* fix

* [Examples] Specify version for vllm cuz vllm v0.6.4.post1 has issue (#4391)

* [OCI] Specify vllm version because the latest vllm v0.6.4.post1 has issue

* version for vllm-flash-attn

* [docs] Specify compartment for OCI resources. (#4384)

* [docs] Specify compartment for OCI resources.

* Add link to compartment definition page

* [k8s] Improve multi-node provisioning time (nimbus) (#4393)

* Tracking k8s events with timeline

* Remove SSH wait

* Parallelize pod creation and status check

* Parallelize labelling, add docs on optimizing base image, bump default provision timeout

* More parallelization, batching and optimizations

* lint

* correctness

* Fix double launch bug

* fix num threads

* Add fd limit warning

* [k8s] Move setup and ray start to pod args to make them async (#4389)

* move scripts to args

* Avoid ray setup

* fix

* Add checks for ray healthiness

* remove bc installation

* wait for healthy

* add todo

* fix

* fix

* format

* format

* remove unnecessary logging

* print out error setup

* Add comment

* clean up the logging

* style

* Fixes for ubuntu images

* format

* remove unused comments

* Optimize ray start

* add comments

* Add comments

* Fix comments and logging

* missing end_epoch

* Add logging

* Longer timeout and trigger ray start

* Fixes for the ray port and AWS credential setup

* Update netcat-openbsd, comments

* _NUM_THREADS rename

* add num_nodes to calculate timeout

* lint

* revert

* use uv for pip install and for venv creation (#4394)

* use uv for pip install and for venv creation

uv is a tool that can replace pip and venv (and some other stuff we're not using
I think). It's written in rust and in testing is significantly faster for many
operation, especially things like `pip list` or `pip install skypilot` when
skypilot or all its dependencies are already installed.

* add comment to SKY_PIP_CMD

* sudo handling for ray

* Add comment in dockerfile

* fix pod checks

* lint

---------

Co-authored-by: Zhanghao Wu <[email protected]>
Co-authored-by: Christopher Cooper <[email protected]>

* [Core] Skip worker ray start for multinode (#4390)

* Optimize ray start

* add comments

* update logging

* remove `uv` from runtime setup due to azure installation issue (#4401)

* [k8s] Skip listing all pods to speed up optimizer (#4398)

* Reduce API calls

* lint

* [k8s] Nimbus backward compatibility (#4400)

* Add nimbus backward compatibility

* add uv backcompat

* add uv backcompat

* add uv backcompat

* lint

* merge

* merge

* [Storage] Call `sync_file_mounts` when either rsync or storage file_mounts are specified  (#4317)

do file mounts if storage is specified

* [k8s] Support in-cluster and kubeconfig auth simultaneously (#4188)

* per-context SA + incluster auth fixes

* lint

* Support both incluster and kubeconfig

* wip

* Ignore kubeconfig when context is not specified, add su, mounting kubeconfig

* lint

* comments

* fix merge issues

* lint

* Fix Spot instance on Azure (#4408)

* [UX] Allow disabling ports in CLI (#4378)

[UX] Allow disabling ports

* [AWS] Get rid of credential files if `remote_identity: SERVICE_ACCOUNT` specified (#4395)

* syntax

* minor

* Fix OD instance on Azure (#4411)

* [UX] Remove K80 and M60 from common GPU list (#4382)

* Remove K80 and M60 from GPU list

* Fix kubernetes instance type with space

* comments

* format

* format

* remove mi25

* Event based smoke tests -- manged jobs (#4386)

* event based smoke test

* more event based smoke test

* more test cases

* more test cases with managed jobs

* bug fix

* bump up seconds

* merge master and resolve conflict

* more test case

* support test_managed_jobs_pipeline_failed_setup

* support test_managed_jobs_recovery_aws

* manged job status

* bug fix

* test managed job cancel

* test_managed_jobs_storage

* more test cases

* resolve pr comment

* private member function

* bug fix

* interface change

* bug fix

* bug fix

* raise error on empty status

* [k8s] Fix in-cluster auth namespace fetching (#4420)

* Fix incluster auth namespace fetching

* Fixes

* [k8s] Update comparison page image (#4415)

Update image

* Add a pre commit config to help format before pushing (#4258)

* pre commit config

* yapf version

* fix

* mypy check all files

* skip smoke_test.py

* add doc

* better format

* newline format

* sync with format.sh

* comment fix

* fix the pylint hook for pre-commit (#4422)

* fix the pylint hook

* remove default arg

* change name

* limit pylint files

* [k8s] Fix resources.image_id backward compatibility (#4425)

* Fix back compat

* Fix back compat for image_id + regions

* lint

* comments

* [Tests] Move tests to uv to speed up the dependency installation by >10x (#4424)

* correct cache for pypi

* Add doc cache and test cache

* Add examples folder

* fix policy path

* use uv for pylint

* Fix azure cli

* disable cache

* use venv

* set venv

* source instead

* rename doc build

* Move to uv

* Fix azure cli

* Add -e

* Update .github/workflows/format.yml

Co-authored-by: Christopher Cooper <[email protected]>

* Update .github/workflows/mypy.yml

Co-authored-by: Christopher Cooper <[email protected]>

* Update .github/workflows/pylint.yml

Co-authored-by: Christopher Cooper <[email protected]>

* Update .github/workflows/pytest.yml

Co-authored-by: Christopher Cooper <[email protected]>

* Update .github/workflows/test-doc-build.yml

Co-authored-by: Christopher Cooper <[email protected]>

* fix pytest yml

* Add merge group

---------

Co-authored-by: Christopher Cooper <[email protected]>

* fix db

* fix launch

* remove transaction id

* format

* format

* format

* test doc build

* doc build

* update readme for test kubernetes example (#4426)

* update readme

* fetch version from gcloud

* rename var to GKE_VERSION

* subnetwork also use REGION

* format

* fix types

* fix

* format

* fix types

* [k8s] Fix `show-gpus` availability map when nvidia drivers are not installed (#4429)

* Fix availability map

* Fix availability map

* fix types

* avoid catching ValueError during failover (#4432)

* avoid catching ValueError during failover

If the cloud api raises ValueError or a subclass of ValueError during instance
termination, we will assume the cluster was downed. Fix this by introducing a
new exception ClusterDoesNotExist that we can catch instead of the more general
ValueError.

* add unit test

* lint

* [Core] Execute setup when `--detach-setup` and no `run` section (#4430)

* Execute setup when --detach-setup and no run section

* Update sky/backends/cloud_vm_ray_backend.py

Co-authored-by: Tian Xia <[email protected]>

* add comments

* Fix types

* format

* minor

* Add test for detach setup only

---------

Co-authored-by: Tian Xia <[email protected]>

* wait for cleanup

* [Jobs] Allow logs for finished jobs and add `sky jobs logs --refresh` for restartin jobs controller (#4380)

* Stream logs for finished jobs

* Allow stream logs for finished jobs

* Read files after the indicator lines

* Add refresh for `sky jobs logs`

* fix log message

* address comments

* Add smoke test

* fix smoke

* fix jobs queue smoke test

* fix storage

* fix merge issue

* fix merge issue

* Fix merging issue

* format

---------

Signed-off-by: nkwangleiGIT <[email protected]>
Co-authored-by: Christopher Cooper <[email protected]>
Co-authored-by: Lei <[email protected]>
Co-authored-by: Romil Bhardwaj <[email protected]>
Co-authored-by: Cody Brownstein <[email protected]>
Co-authored-by: mjibril <[email protected]>
Co-authored-by: zpoint <[email protected]>
Co-authored-by: Hysun He <[email protected]>
Co-authored-by: Tian Xia <[email protected]>
Co-authored-by: zpoint <[email protected]>
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