[chore]: improve public api checks and error reporting - #1096
Conversation
danieldk
left a comment
danieldk
left a comment
There was a problem hiding this comment.
Thanks! Added a bunch of comments.
| def report( | ||
| kernel: str, base: dict, head: dict, limit: int = 20, preview: int = 6 | ||
| ) -> bool: | ||
| ) -> tuple: |
There was a problem hiding this comment.
Tuple typing, especially because the return statement is somewhat hard to read.
There was a problem hiding this comment.
I didn't understand the suggestion here.
There was a problem hiding this comment.
I changed it to a dict which I believe is more readable and easier to parse cognitively.
| _tree(items) | ||
| return True | ||
|
|
||
| grown = [ |
There was a problem hiding this comment.
Isn't there only one __all__? If so, why a list comprehension?
There was a problem hiding this comment.
Would also benefit from some comments. E.g. what is the difference between added and grown?
There was a problem hiding this comment.
scattermoe has more than one:
torch-ext/scattermoe/__init__.pytorch-ext/scattermoe/kernels/__init__.py
| print(f" => version bumped {old_version} -> {new_version}") | ||
| continue | ||
|
|
||
| flavour = additive_safe(head_src) |
There was a problem hiding this comment.
Seeing the call site, why is additive_safe stringly typed?
There was a problem hiding this comment.
What would you prefer? Enum?
There was a problem hiding this comment.
Changed to Enum and I think it reads more explicit.
Co-authored-by: Daniël de Kok <me@danieldk.eu>
|
@danieldk addressed your comments |
drbh
left a comment
drbh
left a comment
There was a problem hiding this comment.
lgtm!
**might be best for Daniel to give another once over before merging in case any of the comments need follow up
3 participants
#1095 bumped the version but the API check still complained. This PR fixes that. Additionally, it broadens the coverage of potential suspects that warrant a version bump as laid down here.
Closes #1030