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

Update intro and flux example #1275

Merged
merged 13 commits into from
Sep 23, 2024
Merged

Update intro and flux example #1275

merged 13 commits into from
Sep 23, 2024

Conversation

py-rh
Copy link
Contributor

@py-rh py-rh commented Sep 20, 2024

No description provided.

docs/index.rst Outdated
@@ -13,6 +11,38 @@
<a class="github-button" href="https://github.com/run-house/runhouse/fork" data-icon="octicon-repo-forked" data-size="large" aria-label="Fork run-house/runhouse on GitHub">Fork</a>
</p>

Why Runhouse?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can drop this or move it below the intro sentence. Page layout look kind of odd when you go straight from H1 to H2 without any intro paragraph. Technically the sentence following is more "what" than "why anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I don't super love it either way somehow lol

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Went down a rabbit hole questioning whether or not I've been thinking of this wrong, but feeling better with Google recommending not to have headers without text - https://developers.google.com/style/headings#heading-and-title-format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM

@@ -0,0 +1,102 @@
import runhouse as rh
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be in /examples or is it just a file to accompany a page in /guides? If the former, I think it needs an intro paragraph and h1 in comments about this line. Potentially a bit odd to start with an import statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guides + Github, not examples.

@@ -25,7 +25,7 @@ make sure our AWS credentials are set up:
$ aws configure
$ sky check
```
We'll be downloading the Llama2 model from Hugging Face, so we need to set up our Hugging Face token:
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@mkandler mkandler left a comment

Choose a reason for hiding this comment

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

Looks good! Comments aren't necessarily blocking but may be worth updating before anything is published on the website.

@@ -0,0 +1,32 @@
# Deploy Flux1 Schnell on AWS EC2

See a more [rich explanation](https://www.run.house/examples/host-and-run-flux1-image-genai-aws)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this isn't going in Examples, then this needs to change ☝️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! Changed to guides

@py-rh py-rh merged commit 7816d66 into main Sep 23, 2024
13 checks passed
@py-rh py-rh deleted the update-intro-and-flux-example branch September 23, 2024 11:59
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