-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Rewrite container docs for custom containers #13420
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
Conversation
7259ff9 to
c9c75ab
Compare
|
R: @tysonjh Current page: https://beam.apache.org/documentation/runtime/environments/ |
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unfortunate. WDYT about Dataflow using this flag as well in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the future there is a plan for Portable runner to support Dataflow endpoint, not sure how far on the roadmap that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tvalentyn Can you send me more information on this plan if you have any?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I'd like to know about this as well since we shouldn't go to GA with custom containers until the flags are settled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless of the plans for using the Portability API for submitting jobs, we should standardize on a single flag for specifying the image across runners. It could be argued that environment_config is not the best flag for this; maybe we should take it to the list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless of the plans for using the Portability API for submitting jobs, we should standardize on a single flag for specifying the image across runners. It could be argued that environment_config is not the best flag for this; maybe we should take it to the list?
+1 I agree that standardizing the flag is a worthwhile improvement, but it's out of scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Standardizing the flag should be done before we document and adveritize this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our plan was to keep these as is for preview and standardize for GA. This is also editing existing documentation which used --environment_config. I can send an email to dev re:flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're supporting old SDKs, we have to support the old flag anyway, so we should go ahead and get this in and standardizing going forward.
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put this into a code block or reformat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The {{< highlight class="runner-X" >}} formats it into code-blocks, tabbed by runner type - see existing page https://beam.apache.org/documentation/runtime/environments/#testing-customized-images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I went to 'view file' in GitHub and it only showed the rendered markdown which made each of these H1 titles. Is there a way to preview?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best way is to serve the website from source: ./gradlew :website:serveWebsite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also view the details of Website_Stage_GCS ("Run Website_Stage_GCS PreCommit") to find the staged version of the website. You can rerun the test if the link's gone stale.
07a91f0 to
96739ba
Compare
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the future there is a plan for Portable runner to support Dataflow endpoint, not sure how far on the roadmap that is.
|
(some of my comments got hidden under |
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
tvalentyn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, feel free to ping me when this is ready to merge.
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
c9c883f to
e1702d8
Compare
ibzib
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
It's exciting to finally have custom containers on Dataflow publicly documented.
rosetn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for rewriting this!
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does "Navigate to the root directory where you've installed your local copy of the Beam SDK." work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to make it clearer (added some env var setting in instructions) - mostly worried installed might get confused for package installation (i.e. where pip installed Beam)
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an anchor to this so I can link to it in cloud docs please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added #running-pipelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo here--I also think you can just call this section "Troubleshooting" or "Considerations"
website/www/site/content/en/documentation/runtime/environments.md
Outdated
Show resolved
Hide resolved
4244d56 to
b7a4fb6
Compare
rosetn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small typo but otherwise LGTM
| COPY /src/path/to/file /dest/path/to/file/ | ||
| ``` | ||
|
|
||
| This `Dockerfile`: uses the prebuilt Python 3.7 SDK container image [`beam_python3.7_sdk`](https://hub.docker.com/r/apache/beam_python3.7_sdk) tagged at (SDK version) `2.25.0`, and adds an additional environment variable and file to the image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra colon after Dockerfile
This rewrites the container guide as we move to launch custom containers for Dataflow runner. There are two methods that should be highlighted - one is using the released Dockerhub images as a base image, and one is building from Beam source itself.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.