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

Introduce NativeLink Web Bridge #1289

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SchahinRohani
Copy link
Contributor

@SchahinRohani SchahinRohani commented Aug 28, 2024

Description

This is a fast prototype for subscribing to the redis/dragonflydb "build_events" channel and
decode them properly via protobuf and fires them via websocket to the browser.

It's a plain prototype without any error handling and formatting

Included: latency calculation from bazel to bridge

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

In 4 different shells inside the nix flake environment on MacOS.

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

@SchahinRohani SchahinRohani changed the title Add NativeLink Web Bridge Introduce NativeLink Web Bridge Aug 30, 2024
@SchahinRohani SchahinRohani marked this pull request as ready for review September 10, 2024 17:51
Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r4, 15 of 15 files at r5, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and 19 of 43 files reviewed, and 10 discussions need to be resolved


web/bridge/.gitignore line 1 at r5 (raw file):

# Logs

nit: Could remove everything from this file that we don't actually use?


web/bridge/.env.example line 3 at r5 (raw file):

REDIS_URL=redis://localhost:6379
NATIVELINK_PUB_SUB_CHANNEL=build_event
POSTGRES_URL=

nit: Consider setting a default postres url


web/bridge/image.nix line 7 at r5 (raw file):

}: let
  description = "A simple Bun environment image";
  title = "Bun Environment";

Let's inline the title and description.


web/bridge/README.md line 73 at r3 (raw file):

Previously, SchahinRohani (Schahin) wrote…

And how can we use a custom .bazelrc with the hello-lre example?

I don't think we'd need one. Instead of the bazelrc we can just putt all these flags into the bazelrc and leave a "tip" mentioning thath the flags can be put into a bazelrc as well. Maybe something like this, similar to how we do it in https://www.nativelink.com/docs/deployment-examples/kubernetes:

bazel clean && bazel build \
    --remote_cache=grpc://$CACHE \
    --remote_executor=grpc://$SCHEDULER \
    --THE_OTHER_FLAGS... \
    //local-remote-execution/examples:hello_lre

web/bridge/src/index.ts line 14 at r5 (raw file):

  // NativeLink URL
  const nativelinkRepo = "TraceMachina/nativelink"
  const nativelinkBranch = "main"

I'm just noticing that we can't use branches like "main" or "master" here, as it would cause unpredictable behavior if the branches got updated. Instead, let's pin all these URLs to specific commits. We can also remove the variables etc. Let's use regular URLs which are easier to copy-paste.


web/bridge/src/protobuf.ts line 6 at r5 (raw file):

    console.log("Loading Remote Proto Files");

    // Create a new Root instance

Remove all the comments that explain trivial lines of code.


web/bridge/src/protobuf.ts line 43 at r5 (raw file):

    // Handle googleapis imports
    if (importPath.startsWith("google/api") || importPath.startsWith("google/devtools/build/v1")) {
        return `https://raw.githubusercontent.com/googleapis/googleapis/master/${importPath}`;

As mentioned above, we'll need to keep these paths globally and use pinned urls.


web/bridge/src/utils.ts line 1 at r5 (raw file):

type ParsedMessage = {

Everything in this file seems to be used just by the eventhandler.ts. Let's move these functions there.


web/bridge/src/websocket.ts line 6 at r5 (raw file):

export const startWebSocket = () => {
  console.log('\nWebSocket server is running on ws://localhost:8080\n');

This probably shouldn't be hardcoded


web/bridge/src/websocket.ts line 8 at r5 (raw file):

  console.log('\nWebSocket server is running on ws://localhost:8080\n');
  Bun.serve({
  port: 8080,

Let's not hardcode ports.

Copy link
Contributor Author

@SchahinRohani SchahinRohani left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained, and 8 of 43 files reviewed, and pending CI: Web Platform Deployment / ubuntu-24.04, pre-commit-checks, and 8 discussions need to be resolved


web/bridge/image.nix line 7 at r5 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Let's inline the title and description.

Done.


web/bridge/README.md line 73 at r3 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I don't think we'd need one. Instead of the bazelrc we can just putt all these flags into the bazelrc and leave a "tip" mentioning thath the flags can be put into a bazelrc as well. Maybe something like this, similar to how we do it in https://www.nativelink.com/docs/deployment-examples/kubernetes:

bazel clean && bazel build \
    --remote_cache=grpc://$CACHE \
    --remote_executor=grpc://$SCHEDULER \
    --THE_OTHER_FLAGS... \
    //local-remote-execution/examples:hello_lre

I've added the full command.


web/bridge/src/index.ts line 14 at r5 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I'm just noticing that we can't use branches like "main" or "master" here, as it would cause unpredictable behavior if the branches got updated. Instead, let's pin all these URLs to specific commits. We can also remove the variables etc. Let's use regular URLs which are easier to copy-paste.

Pinned the bazel repo to the newest tag and the googleapis repo to the newest commit by today.


web/bridge/src/protobuf.ts line 6 at r5 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Remove all the comments that explain trivial lines of code.

Done.


web/bridge/src/protobuf.ts line 43 at r5 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

As mentioned above, we'll need to keep these paths globally and use pinned urls.

Updated to pinned versions.


web/bridge/src/websocket.ts line 6 at r5 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

This probably shouldn't be hardcoded

Done.


web/bridge/src/websocket.ts line 8 at r5 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Let's not hardcode ports.

Ok, moved it out into the .env. An example can be found in .env.example


web/bridge/src/utils.ts line 1 at r5 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Everything in this file seems to be used just by the eventhandler.ts. Let's move these functions there.

Done.

This is a fast prototype for subscribing to the redis/dragonflydb "build_events" channel and
decode them properly via protobuf and fires them via websocket to the browser.
Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Nice work!

:lgtm:

Reviewed 23 of 39 files at r2, 1 of 14 files at r3, 12 of 12 files at r6, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and 1 discussions need to be resolved


web/bridge/src/protobuf.ts line 37 at r6 (raw file):

    if (importPath.startsWith("google/protobuf")) {
        return `https://raw.githubusercontent.com/protocolbuffers/protobuf/v29.0-rc2/src/${importPath}`;

nit: Consider deduplicating these urls between protobuf.ts and index.ts to ensure that both sides use the same versions.

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