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

Tour of beam learning materials CI/CD refactoring and templating #25080

Merged

Conversation

olehborysevych
Copy link
Collaborator

Tour of Beam learning materials refactored to support template learning materials
Introduction module learning materials updated to match new LM structure
resolves #25079


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI.

@olehborysevych
Copy link
Collaborator Author

R: @TSultanov
Hi Timur, could you please make an internal review

@github-actions
Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

sdk:
- Java
- Python
- Go
id: from-memory
name: Creating in-memory PCollections
taskName: ParDo
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add newline to the end

sdk:
- Java
- Python
- Go
id: from-csv
name: Creating PCollections from csv files
taskName: CSV
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add newline to the end

@@ -46,6 +46,7 @@ func getExampleNode(id string, sdk tob.Sdk) tob.Node {
}

func TestSample(t *testing.T) {
//trees, err := CollectLearningTree(filepath.Join("..", "..", "samples", "learning-content"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented out code

@olehborysevych
Copy link
Collaborator Author

R: @damccorm

@olehborysevych olehborysevych marked this pull request as ready for review January 19, 2023 13:44
Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, just had some small feedback

}
node := &tob.Node{}
for _, f := range files {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't technically a problem with this code change, but I think this loop has a potential problem. If the first file has an associated error, but the second does not it will reset err to nil

Probably we should be early returning if err != nil after the switch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you! Nice catch!

@@ -25,6 +25,19 @@ The Direct Runner executes pipelines on your machine and is designed to validate

Using the Direct Runner for testing and development helps ensure that pipelines are robust across different Beam runners. In addition, debugging failed runs can be a non-trivial task when a pipeline executes on a remote cluster. Instead, it is often faster and simpler to perform local unit testing on your pipeline code. Unit testing your pipeline locally also allows you to use your preferred local debugging tools.

{{if (eq .Sdk "go")}}
In the SDK Go, the default is runner **DirectRunner**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In the SDK Go, the default is runner **DirectRunner**.
In the Go SDK, the default is runner **DirectRunner**.

{{if (eq .Sdk "go")}}
In the SDK Go, the default is runner **DirectRunner**.

Additionally, you can read [here](https://beam.apache.org/documentation/runners/direct/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Additionally, you can read [here](https://beam.apache.org/documentation/runners/direct/)
Additionally, you can read more about the Direct Runner [here](https://beam.apache.org/documentation/runners/direct/)

{{end}}

{{if (eq .Sdk "python")}}
In the SDK Python, the default is runner **DirectRunner**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In the SDK Python, the default is runner **DirectRunner**.
In the Python SDK, the default is runner **DirectRunner**.

id: guide
name: Tour of Beam Guide
complexity: BASIC
Copy link
Contributor

Choose a reason for hiding this comment

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

Was removing this line intentional?

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Thanks!

@damccorm
Copy link
Contributor

I'll merge once CI completes

@damccorm damccorm merged commit 3e1291c into apache:master Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tour of Beam] [Task] Refactor learning materials CI/CD to support template content
3 participants