Skip to content

add type annotations and docstrings to devlib #715

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

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

Conversation

arjpur01
Copy link

Most of the files are covered, but some of the
instruments and unused platforms are not augmented Minimum Python version required change from 3.7 to 3.10 in order to support some of the annotation features

@marcbonnici
Copy link
Collaborator

Hi,

If this requires changing the minimum supported Python version that means we need to perform a new release of both devlib and the WA project on github and PyPI before we can merge this PR.

We should also check with our known users what version of Python they are currently using to ensure we provide sufficient time for any users to migrate if required before dropping support for < 3.10.

@arjpur01
Copy link
Author

Hi,

If this requires changing the minimum supported Python version that means we need to perform a new release of both devlib and the WA project on github and PyPI before we can merge this PR.

We should also check with our known users what version of Python they are currently using to ensure we provide sufficient time for any users to migrate if required before dropping support for < 3.10.

Hi Marc,

My changes dont exactly need the 3.10 version. I have modified now, to avoid the explicit dependency on the newer language features, for ex - changing imports of Literal, Protocol and TypedDict from typing_extensions instead of from typing.
I have reverted the setup.py change to move up to 3.10 in this PR.
I have validated on bench to use 3.7, 3.8, 3.9 and 3.10 python versions with this changes.

Still i think it is something we should pursue and move up to 3.10 in near future, but that effort can be taken up independent of this PR.

Kindly review my changes and do the needful.

Thanks and Regards,
Arjun

@arjpur01 arjpur01 force-pushed the master branch 4 times, most recently from b42c7b4 to d2f5bb8 Compare March 18, 2025 12:13
Copy link
Collaborator

@marcbonnici marcbonnici left a comment

Choose a reason for hiding this comment

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

I still need to finish looking through the PR, however I have left some initial comments.

@arjpur01
Copy link
Author

Thanks Marc, I will look into the above and make the changes. meanwhile, please continue to review the code.

Copy link
Collaborator

@douglas-raillard-arm douglas-raillard-arm left a comment

Choose a reason for hiding this comment

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

This review covers:

  • devilb/_target_runner.py
  • devlib/collector/__init__.py
  • devlib/collector/dmesg.py
  • devlib/collector/ftrace.py

Did not have time to review the rest yet, but I'll come back to it for more rounds.

@arjpur01 arjpur01 force-pushed the master branch 3 times, most recently from 0f975c3 to a466f4c Compare May 7, 2025 12:47
@arjpur01 arjpur01 force-pushed the master branch 3 times, most recently from 248f294 to 1151a9b Compare May 12, 2025 13:43
Most of the files are covered, but some of the
instruments and unused platforms are not augmented
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.

3 participants