A few more pyright fixes #23453
A few more pyright fixes #23453
Conversation
superbobry
force-pushed
the
pyrighttoo
branch
from
ccbf9f1
to
133ded4
Compare
superbobry
force-pushed
the
pyrighttoo
branch
from
133ded4
to
6f386dd
Compare
| except FileNotFoundError: | ||
| pass | ||
| if filename in self._file_cache: | ||
| source = self._file_cache[filename] |
There was a problem hiding this comment.
This is a change in program logic – this block now executes even if inspect.getmodulename(filename) is False. I'm not sure of the ramifications of that here: is that an intended change?
There was a problem hiding this comment.
I think this was a bug in the original implementation.
If the filename is already cached (and it could've only been cached if inspect.getmodulename(filename) returned True AFAICT), this method would raise NotImplementedError.
| @@ -1266,6 +1268,7 @@ def check(cond): | |||
| dims = [] | |||
| while flat_assignment.size > 1: | |||
| stride = flat_assignment[1] | |||
| i = 0 | |||
There was a problem hiding this comment.
Can we skip this fix please? Or you can send this as a seperate CL to me. I would have to check if this is correct.
SKipping this fix, is not going to break anything since we don't run pyright in our CI.
There was a problem hiding this comment.
Sure, I can revert, but I'm curious why this can be problematic?
The fix is purely technical: flat_assigments.size > 1 guarantees that the loop will iterate at least one, but there isn't a way to express that in the type system.
There was a problem hiding this comment.
Then send a separate CL please? I want to make sure this is correct and doesn't cause any problems because a lot of code goes via this code.
| @@ -575,6 +576,7 @@ def shard_shape(self, global_shape: Shape) -> Shape: | |||
| f'devices passed to PmapSharding. Got sharded dimension {sharded_dim} ' | |||
| f'with value {global_shape[sharded_dim]} in shape {global_shape} and ' | |||
| f'the number of devices={len(self._device_assignment)}') | |||
| assert sharded_shape is not None | |||
There was a problem hiding this comment.
Why do you need this? Looks like this will always be True?
Can you just add a else branch to error after line 570 or silence pyright here?
There was a problem hiding this comment.
This is only True if s is guaranteed to either be sharding_specs.{Unstacked,Chunk}. Otherwise, it's undefined.
There was a problem hiding this comment.
Won't adding an else branch like I said above silence the error?
No description provided.