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.

Sign up for GitHub

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

Jump to bottom

protobuf: automate derived file generation #6136

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

protobuf: automate derived file generation #6136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ab1ede
protobuf: automate derived file generation
oliver-sanders Jun 7, 2024
c908539
network: move code out of __init__file
oliver-sanders Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .codacy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
exclude_paths:
- 'etc/**'
- 'tests/**'
- 'cylc/flow/**_pb2.py'
- 'cylc/flow/network/protobuf/**_pb2.py'
2 changes: 1 addition & 1 deletion .codecov.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ coverage:
# files to ignore
ignore:
- "tests/**"
- "ws_messages_pb2.py"
- "cylc/flow/network/protobuf/cylc/v5/schema_pb2.py"
- "cylc/flow/scripts/report_timings.py"

flag_management:
Expand Down
4 changes: 2 additions & 2 deletions .coveragerc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ disable_warnings =
module-not-measured
omit =
tests/*
*/cylc/flow/*_pb2.py
*/cylc/flow/network/protobuf/cylc/v5/*_pb2.py
cylc/flow/etc/*
cylc/flow/scripts/report_timings.py
parallel = True
Expand Down Expand Up @@ -43,7 +43,7 @@ fail_under=0
ignore_errors = False
omit =
tests/*
*/cylc/flow/*_pb2.py
*/cylc/flow/network/protobuf/cylc/v5/*_pb2.py
cylc/flow/etc/*
precision = 2
show_missing = False
Expand Down
83 changes: 83 additions & 0 deletions .github/workflows/protobuf.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
name: protobuf
# CI tests to run against the protobuf schema on change:

on:
# run for any PRs raising changes to the protobuf files or setup
pull_request:
paths:
- 'cylc/flow/network/protobuf/**'

jobs:
protobuf:
runs-on: ubuntu-latest
timeout-minutes: 10

steps:
- name: Checkout
uses: actions/checkout@v4
with:
# we need all of the commits on the PR branch in order to be able to add new ones
fetch-depth: 100

- name: Configure git
uses: cylc/release-actions/configure-git@v1

- name: Install Protobuf
uses: mamba-org/setup-micromamba@v1
with:
# install protobuf into a mamba env (note use shell = "bash -el {0}"
# to access this envionment)
environment-name: protobuf
create-args: protobuf
init-shell: bash
Comment on lines +25 to +32
Copy link
Member

Choose a reason for hiding this comment

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

Is there any point using an environment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Conda/Mamba can only install packages into isolated environments, they cannot install into the system environment.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need an environment on the temporary GH Actions runner?

Copy link
Member Author

@oliver-sanders oliver-sanders Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You cannot install software using mamba/conda without using an environment. It is not possible.

Copy link
Member

Choose a reason for hiding this comment

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

I am asking why are you using mamba/conda instead of pip (which is simpler - no bash -el {0} needed)?

Copy link
Member Author

@oliver-sanders oliver-sanders Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Mamba installs low-level dependencies properly, pip doesn't.

I think I had issues installing from pip/npm but it was a while back so I can't quite remember the deal.


- name: Install bufbuild/buf
run: |
eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)" # activate homebrew

# NOTE: bufbuild does exist on conda-forge but hasn't been updated for a while
brew install bufbuild/buf/buf

- name: Lint
run: |
# lint .proto files
eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)" # activate homebrew
cd cylc/flow/network/protobuf
buf lint
Copy link
Member

Choose a reason for hiding this comment

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

(Looks like buf can be installed via npm which might be easier to use in GH Actions, rather than activating homebrew every step)

Btw, first result when I googled "buf" 😆 image

Copy link
Member Author

@oliver-sanders oliver-sanders Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wouldn't have used a system package manager if I could have avoided it (I don't like system package managers), but I can't remember. Possibly to ensure that dependencies (incl Node) are installed correctly?


- name: Compatibility
shell: bash -el {0}
run: |
eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)" # activate homebrew
cd cylc/flow/network/protobuf
# NOTE: there is currently no process for committing a breaking change.
# If a breaking change is needed:
# - Increment the Cylc API version number.
# - Increment the protobuf schema version number to match.
# - Increment the API number filter in cylc-uiserver.
# - Bypass this step of the workflow.
buf breaking \
--against 'https://github.com/cylc/cylc-flow.git#tag=${{ github.base_ref }},subdir=cylc/flow/network/protobuf'

- name: Build
shell: bash -el {0}
run: |
# generate .py and .pyi files from the .proto files
eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)" # activate homebrew
micromamba activate protobuf
cd cylc/flow/network/protobuf
buf generate

- name: Commit & Push
run: |
if [[ -z $(git diff --stat) ]]; then
echo '::error:: No functional changes made to the protobuf schema'
Comment on lines +73 to +74
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if [[ -z $(git diff --stat) ]]; then
echo '::error:: No functional changes made to the protobuf schema'
if git diff --exit-code --stat; then
echo '::warning::No functional changes made to the protobuf schema'

exit 0
else
echo '::info:: pushing update commit'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo '::info:: pushing update commit'
echo '::notice::pushing update commit'

git add -u
git commit -m 'protobuf: updating generated files'
git remote add pr https://github.com/${{ github.event.pull_request.head.repo.owner.login }}/cylc-flow
git push pr HEAD:${{ github.head_ref }}
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I'm not sure this will work for a PR from a fork ☹️

I think GITHUB_TOKEN only has read permissions in PRs from forks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dammit (although the workflow would work on their fork).

Only one way to find out. My master branch is still @ f3342c2, maybe try raising a PR there (along the lines of the one in the OP) and see what happens?

I know there are other repos that manage to do this. E.G. checkout jupyter-server#1448 (deliberately not linking their issue), those pre-commit commits are pushed onto the branch automatically.

Copy link
Member

Choose a reason for hiding this comment

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

https://togithub.com/jupyter-server/jupyter_server/pull/1448

Copy link
Member

Choose a reason for hiding this comment

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

I think that is done by an app rather than GH Actions: https://github.com/marketplace/pre-commit-ci

Copy link
Member

Choose a reason for hiding this comment

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

Didn't work ☹️ oliver-sanders#78 (comment)

exit 0
fi
100 changes: 0 additions & 100 deletions cylc/flow/data_messages_pb2.py
View file
Open in desktop

This file was deleted.

41 changes: 28 additions & 13 deletions cylc/flow/data_store_mgr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,25 @@

from cylc.flow import __version__ as CYLC_VERSION, LOG
from cylc.flow.cycling.loader import get_point
from cylc.flow.data_messages_pb2 import (
PbEdge, PbEntireWorkflow, PbFamily, PbFamilyProxy, PbJob, PbTask,
PbTaskProxy, PbWorkflow, PbRuntime, AllDeltas, EDeltas, FDeltas,
FPDeltas, JDeltas, TDeltas, TPDeltas, WDeltas)
from cylc.flow.network.protobuf.cylc.v5.schema_pb2 import (
PbEdge,
PbEntireWorkflow,
PbFamily,
PbFamilyProxy,
PbJob,
PbTask,
PbTaskProxy,
PbWorkflow,
PbRuntime,
AllDeltas,
EDeltas,
FDeltas,
FPDeltas,
JDeltas,
TDeltas,
TPDeltas,
WDeltas,
)
from cylc.flow.exceptions import WorkflowConfigError
from cylc.flow.id import Tokens
from cylc.flow.network import API
Expand Down Expand Up @@ -382,7 +397,7 @@ def create_delta_store(delta=None, workflow_id=None):
"""Create a mini data-store out of the all deltas message.

Args:
delta (cylc.flow.data_messages_pb2.AllDeltas):
delta (AllDeltas):
The message of accumulated deltas for publish/push.
workflow_id (str):
The workflow ID.
Expand Down Expand Up @@ -430,18 +445,18 @@ class DataStoreMgr:
Local store of config.get_first_parent_ancestors()
.data (dict):
.edges (dict):
cylc.flow.data_messages_pb2.PbEdge by internal ID.
PbEdge by internal ID.
.families (dict):
cylc.flow.data_messages_pb2.PbFamily by name (internal ID).
PbFamily by name (internal ID).
.family_proxies (dict):
cylc.flow.data_messages_pb2.PbFamilyProxy by internal ID.
PbFamilyProxy by internal ID.
.jobs (dict):
cylc.flow.data_messages_pb2.PbJob by internal ID.
PbJob by internal ID.
.tasks (dict):
cylc.flow.data_messages_pb2.PbTask by name (internal ID).
PbTask by name (internal ID).
.task_proxies (dict):
cylc.flow.data_messages_pb2.PbTaskProxy by internal ID.
.workflow (cylc.flow.data_messages_pb2.PbWorkflow)
PbTaskProxy by internal ID.
.workflow (PbWorkflow)
Message containing the global information of the workflow.
.descendants (dict):
Local store of config.get_first_parent_descendants()
Expand Down Expand Up @@ -2688,7 +2703,7 @@ def get_entire_workflow(self):
"""Gather data elements into single Protobuf message.

Returns:
cylc.flow.data_messages_pb2.PbEntireWorkflow
PbEntireWorkflow

"""

Expand Down
Loading
Loading