Repository navigation
NXP backend: refactor dataset_creators to enable better benchmarking - #23886
novak-vaclav wants to merge 1 commit into
Conversation
novak-vaclav
added
module: nxp
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/23886
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit d1df03f with merge base 86b967e ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
|
bf9e2f4 to
8d706ee
Compare
8d706ee to
4c579ef
Compare
4c579ef to
f6daca9
Compare
f6daca9 to
43df070
Compare
|
I initially created this PR and did not assign reviewers because I wanted to double check something, while letting the OSS CI run (in draft mode, CI does not run). Rebased onto current main, ready for review by everyone 😄 |
roman-janik-nxp
left a comment
roman-janik-nxp
left a comment
There was a problem hiding this comment.
There are a lot of failing tests.
| class BaseDatasetCreator(abc.ABC): | ||
| """Abstract base class for all dataset creators.""" | ||
|
|
||
| def __init__(self) -> None: |
There was a problem hiding this comment.
Please don't add type hints -> None. They are meaningless. What I meant in the previous comment was to add type hints for function parameters where it increased clarity, not to add it always everywhere.
There was a problem hiding this comment.
I disagree with them being meaningless. Since we don't enforce strict typing, if -> None is omitted, we can't distinguish whether the method/function returns None, or is missing the typing. So imo it also increases clarity. We sometimes use -> None, and other backends use it as well, and the presence of -> None does not cause any harm imo.
43df070 to
35d9ef3
Compare
35d9ef3 to
338d209
Compare
|
Fixed test issues and rebased onto main |
338d209 to
d1df03f
Compare
Summary
Refactor dataset creators of NXP backend to make the benchmarking code more maintainable. With that, I also implemented changes to make sharing the results of benchmarks better to NXP's internal testers.
Test plan
tests can be manually run using
pytest -c /dev/null backends/nxp/tests/cc @robert-kalmar @JakeStevens @digantdesai @rascani