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

fix(agw): Address mypy errors in lte/gateway/python/ #14889

Merged
merged 2 commits into from
Jan 30, 2023

Conversation

voisey
Copy link
Contributor

@voisey voisey commented Jan 25, 2023

Summary

Fixes all 60 mypy errors in lte/gateway/python. The errors can be looked at locally by running cd $MAGMA_ROOT && mypy lte/gateway/python/. For this, I have been using mypy version 0.991, which is the same as is running in the CI.

Note that in pipelined_test_util.py, I converted SubTest from a named tuple into a dataclass, following the discussion here.

Test Plan

  • Unit tests are green locally: bazel test //lte/gateway/python/...
  • Sudo tests are green locally: bazel/scripts/run_sudo_tests.sh
  • CI

@pull-request-size pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label Jan 25, 2023
@github-actions
Copy link
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

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

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added the component: agw Access gateway-related issue label Jan 25, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 25, 2023

FeG Lint & Test

    2 files  203 suites   40s ⏱️
374 tests 374 ✔️ 0 💤 0
388 runs  388 ✔️ 0 💤 0

Results for commit be10a07.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 25, 2023

DP Lint & Test

14 tests   14 ✔️  2m 10s ⏱️
  1 suites    0 💤
  1 files      0

Results for commit be10a07.

♻️ This comment has been updated with latest results.

@voisey voisey marked this pull request as ready for review January 25, 2023 14:36
@voisey voisey requested a review from a team as a code owner January 25, 2023 14:36
@voisey voisey self-assigned this Jan 25, 2023
@nstng nstng self-requested a review January 27, 2023 10:36
@nstng
Copy link
Contributor

nstng commented Jan 27, 2023

overall lgtml. when I run mypy locally I still get one error

~/magma$ pip3 show mypy
Name: mypy
Version: 0.991

~/magma$ python3 -m mypy lte/gateway/python/
...
lte/gateway/python/integ_tests/gateway/rpc.py:20: error: Module "orc8r.protos.mconfig" has no attribute "mconfigs_pb2"  [attr-defined]
...
Found 1 error in 1 file (checked 674 source files)

Any idea for this? There are a lot of proto imports that are not flagged.

@nstng
Copy link
Contributor

nstng commented Jan 27, 2023

Any idea for this? There are a lot of proto imports that are not flagged.

Looked into this offline in pairing. Looks like mypy (and python) think the import is working because there is an (unrelated) folder orc8r/protos/mconfig without any py files -> something like an empty import.
This should be excluded from mypy checks with a respective comment.

Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>
@github-actions
Copy link
Contributor

github-actions bot commented Jan 30, 2023

Oops! Looks like you failed the AGW Build & Format Python.

Howto

♻️ Updated: ✅ The check is passing the AGW Build & Format Python after the last commit.

@voisey
Copy link
Contributor Author

voisey commented Jan 30, 2023

Thanks for flagging this @nstng. For some reason that error only shows up for me sometimes, so I've excluded it for now. The same import is used several times in orc8r/gateway/python, but I don't see mypy errors for these cases when I run mypy orc8r/gateway/python. However, I do get errors in some cases when I run mypy on the specific files, but not always... I propose getting this PR merged and then investigating this further with a potential follow-up. Does that sound okay to you?

Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>
@nstng
Copy link
Contributor

nstng commented Jan 30, 2023

I propose getting this PR merged and then investigating this further with a potential follow-up. Does that sound okay to you?

Yes, sounds good. Checked the exclude -> no more errors for me :)

@voisey voisey merged commit c8c2a81 into magma:master Jan 30, 2023
lucasgonze pushed a commit to lucasgonze/magma that referenced this pull request Feb 29, 2024
* fix(agw): Address mypy errors in lte/gateway/python/

Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>

* fix(agw): Ignore protobuf import in mypy

Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>

---------

Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: agw Access gateway-related issue size/L Denotes a Pull Request that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants