chore(pipelined): Make mypy green #12950

sebathomas · 2022-06-09T15:47:19Z

Summary

Fix various type issues for pipelined.

Correct a few wrong argument or variable types
Add type stubs for mixins
Ignore a false positive
Raise ValueError instead of NPE
Extract method to handle all types of error in tc_ops_pyroute2
Add null checks in test code

Test Plan

cd $MAGMA_ROOT/lte/gateway
mypy --ignore-missing-imports python/magma/pipelined/

Success: no issues found in 131 source files

Additional Information

This change is backwards-breaking

github-actions · 2022-06-09T15:48:08Z

Thanks for opening a PR! 💯

A couple initial guidelines

All commits must be signed off. This is enforced by DCO check.
All PR titles must follow the semantic commits format. This is enforced by Semantic PR.

Howto

Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
Checks. All required CI checks must pass before merge.
Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

Contributing Conventions for norms around contributed code

If this is your first Magma PR, also consider reading

Developer Onboarding for onboarding as a new Magma developer
Development Workflow for guidance on your first pull request
CI Checks for points of contact for failing or flaky CI checks
GitHub-to-Slack mappings for Magma maintainers for guidance on how to contact maintainers on Slack

sebathomas · 2022-06-09T15:49:19Z

lte/gateway/python/magma/pipelined/encoding.py

@@ -137,7 +137,7 @@ def get_hash(s, hash_function) -> bytes:

 def encode_str(s: str, encoding_type) -> str:
    if encoding_type == PipelineD.HEConfig.BASE64:
-        s = codecs.encode(codecs.decode(s, 'hex'), 'base64').decode()
+        s = codecs.encode(codecs.decode(s, 'hex'), 'base64').decode()  # type: ignore


This seems to be an issue in mypy, see: python/typeshed#300

github-actions · 2022-06-09T15:55:57Z

feg-workflow

    2 files 202 suites 39s ⏱️
371 tests 371 ✔️ 0 💤 0 ❌
385 runs 385 ✔️ 0 💤 0 ❌

Results for commit 64290b3.

♻️ This comment has been updated with latest results.

github-actions · 2022-06-09T15:58:00Z

dp-workflow

18 tests 18 ✔️ 5m 1s ⏱️
  2 suites   0 💤
  2 files   0 ❌

Results for commit 64290b3.

♻️ This comment has been updated with latest results.

github-actions · 2022-06-09T16:56:10Z

agw-workflow

    78 files   123 suites 6m 34s ⏱️
1 152 tests 1 148 ✔️ 4 💤 0 ❌
1 154 runs 1 150 ✔️ 4 💤 0 ❌

Results for commit 64290b3.

♻️ This comment has been updated with latest results.

lte/gateway/python/magma/pipelined/app/BUILD.bazel

nstng · 2022-06-10T10:57:26Z

lte/gateway/python/magma/pipelined/app/BUILD.bazel

@@ -204,6 +204,9 @@ py_library(
 py_library(
    name = "restart_mixin",
    srcs = ["restart_mixin.py"],
+    deps = [


Curious, why was this not needed before? I see that you added the import to restart_mixin, but the _startup_flow_controller was there before, right?

The import from magma.pipelined.app.startup_flows import StartupFlows was newly added

I see that you added the import to restart_mixin

My question was, why the import was not needed before.

To me it looks like both lines are new?

Current master does this:

but the _startup_flow_controller was there before

yes, but the object that contains the StartupFlows was existing before

Yes but it wasn't typed here before so the Python code didn't know what it was in the abstract superclass.

nstng

lgtm (feel free to ignore the nit if you do not plan to do other changes). sudo tests locally green.

lte/gateway/python/magma/pipelined/ebpf/ebpf_manager.py

lte/gateway/python/magma/pipelined/app/restart_mixin.py

Fix various type issues for pipelined. - Correct a few wrong argument or variable types - Add type stubs for mixins - Ignore some false positives - Raise ValueError instead of NPE - Extract method to handle all types of error in tc_ops_pyroute2 - Add null checks in test code Signed-off-by: Sebastian Thomas <sebastian.thomas@tngtech.com>

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Jun 9, 2022

github-actions bot added the component: agw Access gateway-related issue label Jun 9, 2022

sebathomas commented Jun 9, 2022

View reviewed changes

sebathomas marked this pull request as ready for review June 9, 2022 15:50

sebathomas requested review from a team and koolzz June 9, 2022 15:50

sebathomas force-pushed the pipelined-mypy-green branch 2 times, most recently from 49c149f to 0e1e2ca Compare June 10, 2022 07:19

sebathomas requested review from a team and electronjoe June 10, 2022 07:19

sebathomas force-pushed the pipelined-mypy-green branch from 0e1e2ca to 6f4740e Compare June 10, 2022 07:38

LKreutzer requested review from LKreutzer and removed request for electronjoe June 10, 2022 08:00

nstng reviewed Jun 10, 2022

View reviewed changes

lte/gateway/python/magma/pipelined/app/BUILD.bazel Outdated Show resolved Hide resolved

nstng reviewed Jun 10, 2022

View reviewed changes

sebathomas force-pushed the pipelined-mypy-green branch from 6f4740e to 36f59e5 Compare June 10, 2022 11:47

pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. and removed size/M Denotes a PR that changes 30-99 lines. labels Jun 10, 2022

nstng self-requested a review June 10, 2022 12:18

nstng approved these changes Jun 10, 2022

View reviewed changes

voisey reviewed Jun 10, 2022

View reviewed changes

lte/gateway/python/magma/pipelined/ebpf/ebpf_manager.py Show resolved Hide resolved

voisey reviewed Jun 10, 2022

View reviewed changes

lte/gateway/python/magma/pipelined/app/restart_mixin.py Outdated Show resolved Hide resolved

sebathomas force-pushed the pipelined-mypy-green branch from 36f59e5 to 64290b3 Compare June 10, 2022 13:47

voisey approved these changes Jun 10, 2022

View reviewed changes

sebathomas enabled auto-merge (squash) June 10, 2022 14:40

sebathomas merged commit 1abd2ae into magma:master Jun 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(pipelined): Make mypy green #12950

chore(pipelined): Make mypy green #12950

sebathomas commented Jun 9, 2022 •

edited

github-actions bot commented Jun 9, 2022

sebathomas Jun 9, 2022

github-actions bot commented Jun 9, 2022 •

edited

github-actions bot commented Jun 9, 2022 •

edited

github-actions bot commented Jun 9, 2022 •

edited

nstng Jun 10, 2022

LKreutzer Jun 10, 2022

nstng Jun 10, 2022

LKreutzer Jun 10, 2022

LKreutzer Jun 10, 2022

nstng Jun 10, 2022

sebathomas Jun 10, 2022

nstng left a comment

chore(pipelined): Make mypy green #12950

chore(pipelined): Make mypy green #12950

Conversation

sebathomas commented Jun 9, 2022 • edited

Summary

Test Plan

Additional Information

github-actions bot commented Jun 9, 2022

Howto

More info

Choose a reason for hiding this comment

github-actions bot commented Jun 9, 2022 • edited

feg-workflow

github-actions bot commented Jun 9, 2022 • edited

dp-workflow

github-actions bot commented Jun 9, 2022 • edited

agw-workflow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nstng left a comment

Choose a reason for hiding this comment

sebathomas commented Jun 9, 2022 •

edited

github-actions bot commented Jun 9, 2022 •

edited

github-actions bot commented Jun 9, 2022 •

edited

github-actions bot commented Jun 9, 2022 •

edited