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

Add some type hints to src/microprobe/target folder #42

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

patrick-rivos
Copy link
Contributor

Commit 1: Run the yapf command from utils/ensure_style.sh on the /code folder

Commit 2: Add basic type hints to files in the /code folder.
This includes common refactoring like class(abc.ABC):, removing empty docstrings, **kwargs, adding some TODOs, @abc.abstractproperty -> @property @abc.abstractmethod, etc.

This is a best-effort start to add a baseline since there is so much code in this folder. As more of the project gets annotated, I'll go back through and use the other type hints to add more types to this folder.

Signed-off-by: Patrick O'Neill <patrick@rivosinc.com>
@patrick-rivos patrick-rivos force-pushed the target-type-hints branch 7 times, most recently from 36f2a9a to 9a2ae76 Compare July 10, 2023 23:01
@patrick-rivos
Copy link
Contributor Author

I'm having trouble reproducing the pylint circular-import error locally.
I'm nearly certain that it's due to pylint not handling if TYPE_CHECKING: properly since to my knowledge I didn't add any imports outside of those blocks. I tried adding annotations to ignore the error within the TYPE_CHECKING block, but without a way to reproduce this locally, debugging is a bit like guesswork :(
I'll revisit this in a few days when I have a bit more time.

@rbertran
Copy link
Collaborator

I'm not able to reproduce whatever error you are getting. That said, the CI log (https://api.travis-ci.com/v3/job/605783149/log.txt) shows some errors. Look for [E pattern.

Meanwhile, can you share the details of your setup to enable the type checking ? It'd be good to (eventually) have that enabled by default in the CI tests.

@patrick-rivos
Copy link
Contributor Author

Thanks for the pointer, I was looking at that log but mistook the last 4 lines as errors.

targets/generic/tools/mp_bin2asm.py:1: [R0401(cyclic-import), ] Cyclic import (microprobe -> microprobe.utils.config -> microprobe.utils.logger)
targets/generic/tools/mp_bin2asm.py:1: [R0401(cyclic-import), ] Cyclic import (microprobe.code.ins -> microprobe.utils.asm)
targets/generic/tools/mp_bin2asm.py:1: [R0401(cyclic-import), ] Cyclic import (microprobe.code.ins -> microprobe.utils.asm -> microprobe.utils.bin)
targets/generic/tools/mp_bin2asm.py:1: [R0401(cyclic-import), ] Cyclic import (microprobe.target.uarch -> microprobe.target.uarch.cache -> microprobe.utils.cmdline)

Currently type checking is done using Mypy, which is a static analyzer. I have strict type checking on in my IDE and try to minimize the number of red squiggly lines 😉.

There's also the option to use annotations from a library like typeguard to perform runtime type checking, but I'm not clear what the performance penalty would be like. It might be possible to use it when developing, and then suppress the annotation (link).

Signed-off-by: Patrick O'Neill <patrick@rivosinc.com>
@patrick-rivos
Copy link
Contributor Author

The pylint issues were introduced when migrating from six.with_metaclass(abc.ABCMeta, object) -> abc.ABCMeta. I fixed it by transitioning to abc.ABC instead. Thanks again for the pointer!

@rbertran rbertran mentioned this pull request Jul 12, 2023
13 tasks
@rbertran rbertran merged commit 65cd0c4 into IBM:master Jul 12, 2023
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