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.

Sign up for GitHub

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

Jump to bottom

refactor: 💡 update faq and add more detailed error message #2594

Merged
merged 2 commits into from
Oct 10, 2023
Merged

refactor: 💡 update faq and add more detailed error message #2594

merged 2 commits into from
Oct 10, 2023

Conversation

sunny0826
Copy link
Contributor

More detailed FAQ documentation is available. It also throws out more detailed information when encountering relevant errors to better guide the user in troubleshooting.

Related Issue/PR: #2562 #2547 #2548 #2549 #2555

concretevitamin
concretevitamin reviewed Sep 22, 2023
View reviewed changes
Copy link
Collaborator

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

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

Thanks @sunny0826. A nit.

sky/clouds/service_catalog/common.py Outdated Show resolved Hide resolved
docs/source/reference/faq.rst
Comment on lines +112 to +113
# Fetch U.S. zones for GCP, excluding the specified zones
python -m sky.clouds.service_catalog.data_fetchers.fetch_gcp --exclude us-east1-a us-east1-b
Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed the previous PRs. What's the real use case/motivation for excluding some zones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, I completed that a couple days ago. more info #2562

Copy link
Collaborator

Choose a reason for hiding this comment

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

Checked #2562, didn't seem to find the motivation behind excluding certain zones. Do you mind elaborating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, the --exclude flag was added so that when a problem like #2548 is encountered, it can be quickly excluded without modifying the script.
BTW, it is now recommended to use --zones to select the specified zones instead of getting all zones, which is more efficient and less likely to cause problems.

@sunny0826
Copy link
Contributor Author

sunny0826 commented Sep 23, 2023 •
edited

Launch . /format.sh to got the following output. So I truncated the code line.

./format.sh
SkyPilot Black:
All done! ✨ 🍰 ✨
18 files left unchanged.
SkyPilot yapf: Done
SkyPilot isort:
Skipped 17 files
SkyPilot mypy:
Success: no issues found in 143 source files
Sky Pylint:
************* Module sky.clouds.service_catalog.common
sky/clouds/service_catalog/common.py:199:0: C0301: Line too long (83/80) (line-too-long)
sky/clouds/service_catalog/common.py:200:0: C0301: Line too long (92/80) (line-too-long)

------------------------------------
Your code has been rated at 10.00/10

@sunny0826
Copy link
Contributor Author

Hi @concretevitamin , I've made a note based on your last comment. If you have time, could you look at it again?

concretevitamin
concretevitamin approved these changes Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for the delay.

@concretevitamin concretevitamin merged commit c501143 into skypilot-org:master Oct 10, 2023
18 checks passed
jc9123 pushed a commit to jc9123/skypilot that referenced this pull request Oct 11, 2023
@sunny0826 @jc9123
refactor: 💡 update faq and add more detailed error message (skypilot-…
e00a3f9 
…org#2594)

* refactor: 💡 update faq and add more detailed error message

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@concretevitamin concretevitamin concretevitamin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sunny0826 @concretevitamin