[Tour of Beam][Frontend] Content Tree and SDK models (#23316) #23417

nausharipov · 2022-09-29T08:16:23Z

Resolves #23316

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

Choose reviewer(s) and mention them in a comment (R: @username).
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)

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

models from JSON (apache#23316) removed generated files (apache#23316) const model constructors (apache#23316) fvm config (apache#23316) ignore *g.dart files (apache#23316) server node, group, unit models abstract node model (apache#23316) node type in enums (apache#23316) gitignore sort (apache#23316) sdk model (apache#23316) api calls (apache#23316) getSdks in SdkDropdown (apache#23316) requests in functions (apache#23316) k in functions (apache#23316) server folder in models (apache#23316) review comments (apache#23316) Move response model files (apache#23316) Add CloudFunctionsTobClient (apache#23316) Move a file (apache#23316) Rename a file (apache#23316) ContentTreeCache (apache#23316) ContentTreeCache (apache#23316)

github-actions · 2022-09-29T08:44:56Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damccorm for label build.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

alexeyinkin

LGTM (internal review).
A WIP discussion was in akvelon#263

nausharipov · 2022-09-29T09:01:19Z

R: @damondouglas

github-actions · 2022-09-29T09:02:32Z

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

damondouglas

Additionally, may we add this to the README to instruct others to run, since it deviates from the typical flutter run -d ... command?

flutter packages pub run build_runner build

Essentially it would be great if someone cloning this repository fresh, without any knowledge, could follow the README and convention to run and test.

Finally, as we discussed in the meeting, if we could reposition the RunOrCancelButton at https://github.com/akvelon/beam/blob/tob-23316-rebase/learning/tour-of-beam/frontend/lib/pages/tour/playground_demo.dart#L119, that would be great as well.

damondouglas · 2022-09-30T18:14:09Z

learning/tour-of-beam/frontend/lib/repositories/client/client.dart

+import '../../models/content_tree.dart';
+import '../models/get_sdks_response.dart';
+
+abstract class TobClient {


I like that we are using an abstract class to encapsulate connection to the backend service. However, is there any way we can follow the same client/service model we use with the current Beam playground?

We do have the same entity caches and clients in Playground. The difference is that in Playground we additionally have a repository layer between an entity cache and the abstract network client. It is a useful abstract for an entity cache to not be aware if it gets objects from the network or from a local DB. However, network is the only implementation in Playground so far, and until we have two implementations it's better to not add abstractions. For example, in Playground we have CodeRespository which has runCode method. It should be an action and not a repository intent, but it was modeled after ExampleRepository to just have the same number of abstractions. If we need a layer between an entity cache and the network, I would rather wait for a hands-on with local storage before adding an extra layer here just to see what are the differences in requirements for the implementations.

So I would rather make Playground closer to the current model of ToB.

I'm referring to either gRPC, currently used by Beam Playground or an Open API specification contract between the service and client.

We rejected gRPC in favor of cloud functions for the backend, and it is in the TDD.

As for OpenAPI, we can technically generate Dart files with something like https://pub.dev/packages/openapi_generator
if we had the spec from the backend team.

@eantyshev is it possible to make an OpenAPI spec for these functions?

IMHO OpenApi spec is good when supported with code-generated handlers and models.
We don't have that on backend side, but we provide a README that exposes API samples

AFAIK the native Go way to keep API models is like this: https://github.com/apache/beam/blob/master/learning/tour-of-beam/backend/internal/entity.go

To summarize, we could have an OpenAPI spec to generate frontend files from it. But backend is built in another way, and its code cannot be generated from OpenAPI.

So we have 3 locations to maintain that need to agree on the API:

The backend code itself.

The README of the backend.

The code in the frontend.

If we introduce OpenAPI spec, the 3rd location will change:

The backend code itself.

The README of the backend.

The OpenAPI YAML spec.

But it's still three locations, the 1st location is not going anywhere.

Thank you for thinking through this so carefully.

damondouglas · 2022-09-30T18:14:57Z

learning/tour-of-beam/frontend/lib/repositories/client/cloud_functions_client.dart

+  Future<GetSdksResponse> getSdks() async {
+    final json = await http.get(
+      Uri.parse(
+        'https://us-central1-tour-of-beam-2.cloudfunctions.net/getSdkList',


May we consider not hard-coding URLs in the codebase?

Sure. It's just a temporary URL of a test backend.

In production, we will have a deployment workflow, and the production backend URL will be emitted at some point during that workflow. When it is ready, we will decide how to capture it for the frontend. It may be a generated .dart file with constants (if the URL is more or less permanent) or a text file read at startup (if the URL may change with backend re-deployments).

Without knowing the future infrastructure details, the best we can do is to extract the domain us-central1-tour-of-beam-2.cloudfunctions.net to some constant to not repeat it for two functions which is a too minor improvement. I suggest we do this in the next ToB PR which is already based on this unmerged one.

In this PR, could we see how a configuration would solve preventing this hard coded value?

I made config.dart as a temporary solution.
It now has:

Cloud functions URL which is a function of the project region and the project ID. These both will be known when deploying cloud functions. When that workflow is ready, we will generate a config with those constants.

Playground backend URLs. These are permanent but they are also known when deploying the backend. When Playground backend deployment workflow is fixed, we will be generating both Playground's config an ToB's config.

damondouglas · 2022-09-30T18:15:16Z

learning/tour-of-beam/frontend/lib/repositories/client/cloud_functions_client.dart

+  Future<ContentTreeModel> getContentTree() async {
+    final json = await http.get(
+      Uri.parse(
+        'https://us-central1-tour-of-beam-2.cloudfunctions.net/getContentTree?sdk=Python',


May we consider not hard-coding URLs in the codebase?

damondouglas · 2022-09-30T18:36:23Z

learning/tour-of-beam/frontend/lib/repositories/models/group.dart

+
+import 'node.dart';
+
+part 'group.g.dart';


May we version control this file? For example, the official flutter samples shows version controlling the generated json_serialization along with the base class:
https://github.com/flutter/samples/tree/main/jsonexample/lib/json_serializable

That link shows json_serializable's own examples, and they are version-controlled as an example for the people to see what is generated.

In practice, it is rare useful to version-control generated files because:

They can accidentally get behind their originals or get edited manually. Then mere fixing the generation may break the project. See [Playground Task]: Sort out code generation for constants in frontend #21925

They make diffs larger.

In our particular setting this adds a burden of maintaining the Apache license comments in those files because it gets deleted all the time, and it was painful with Playground generated test mocks until we got rid of those files in the repository. To set if off, we can add an exception for ./gradlew rat, but that just adds things to maintain.

We aim to eliminate generated files from version control. We are almost done with this on the frontend with the only controlled files are gRPC protobuf artifacts.

This makes a lot of sense now. Thank you for clarifying!

I would just advocate to consider an audience who doesn't have the history of decisions to be able to follow a step-by-step instruction to execute in their chosen environment, whether that is local machine or Google Cloud project, for example.

I just saw that you are accomplishing with this akvelon:pg_23304_beam-symbols Thank you :)

I changed my mind towards version-controlling the generated code after reflecting on your point of a barrier to newcomers.

I see one more reason to version-control such files. We had been expecting the programmers to have Flutter for generation which is OK. But we also start expecting them to have Python for Beam symbols extraction which is not that reasonable. When we start extracting symbols for other languages, we will likely be relying on users having other compilers installed which will be totally wrong.

So I am thinking of such a plan:

Version-control all generated files.

Exclude all generated files from Apache Rat check (this really is the most painful thing with their version-controlling as we had been adding licenses manually there).

When building a Docker container, delete and re-generate all generated files so that their manual modifications have no effect in deployment and we see errors stemming from that early.

Add a Gradle task to delete all generated files so that generation is easy to verify and we can test the generated files' consistency before deployment.

Delete and re-generate all generated files in :playground:frontend:precommit

Add a GitHub post-commit workflow to re-generate files and to compare them with the committed versions. Break if a difference is found. This way the committer will receive an email and will soon make it a habit to run the local pre-commit task.

So the only remaining downside is larger diffs.

What do you say?

I agree and thank you again for listening.

alexeyinkin · 2022-10-03T07:26:22Z

Added a simple manual to README.

#23378 comes with Gradle tasks that make building and testing even simpler. When they are approved, we will make similar ones here. So not expanding on testing for now.

alexeyinkin · 2022-10-03T07:29:21Z

Friday's demo is my local WIP, it is not in this PR. So we will move the Run button there, not here.

damondouglas

@damccorm LGTM

github-actions bot added build learning labels Sep 29, 2022

github-actions bot added the Next Action: Reviewers label Sep 29, 2022

alexeyinkin approved these changes Sep 29, 2022

View reviewed changes

damondouglas requested changes Sep 30, 2022

View reviewed changes

Update README with building manual (apache#23316)

2495eba

Move hardcoded URLs to a config file (apache#23316)

107634e

damondouglas approved these changes Oct 6, 2022

View reviewed changes

damccorm merged commit 41ddc4a into apache:master Oct 6, 2022

nausharipov deleted the tob-23316-rebase branch March 28, 2023 10:29


		import 'node.dart';

		part 'group.g.dart';

[Tour of Beam][Frontend] Content Tree and SDK models (#23316) #23417

[Tour of Beam][Frontend] Content Tree and SDK models (#23316) #23417

Uh oh!

Conversation

nausharipov commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GitHub Actions Tests Status (on master branch)

Uh oh!

github-actions bot commented Sep 29, 2022

Uh oh!

alexeyinkin left a comment

Choose a reason for hiding this comment

Uh oh!

nausharipov commented Sep 29, 2022

Uh oh!

github-actions bot commented Sep 29, 2022

Uh oh!

damondouglas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexeyinkin commented Oct 3, 2022

Uh oh!

alexeyinkin commented Oct 3, 2022

Uh oh!

damondouglas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nausharipov commented Sep 29, 2022 •

edited

Loading

damondouglas left a comment •

edited

Loading