Improve typing: better type hints, increased type safety, mypy support #534
Conversation
Codecov Report
@@ Coverage Diff @@
## master #534 +/- ##
==========================================
+ Coverage 97.26% 97.43% +0.16%
==========================================
Files 88 88
Lines 8116 8375 +259
==========================================
+ Hits 7894 8160 +266
+ Misses 222 215 -7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Co-authored-by: Adam Gleave <adam@gleave.me>
Co-authored-by: Adam Gleave <adam@gleave.me>
Co-authored-by: Adam Gleave <adam@gleave.me>
There was a problem hiding this comment.
LGTM! There's a couple minor changes outlined in comments, most substantive of one is avoiding adding some spurious "output": [] in notebooks (I think spurious -- if there's a good reason for it happy to change my mind). But I don't think this needs my review again unless you end up making substantial changes.
Thanks for seeing this PR through to the end :)
ci/clean_notebooks.py
Outdated
| if cell["cell_type"] == "code" and not cell["source"]: | ||
| if check_only: | ||
| raise UncleanNotebookError(f"Notebook {file} has empty code cell") | ||
| nb.cells.remove(cell) | ||
| was_dirty = True | ||
| for field, default in fields_defaults: | ||
| if cell.get(field) != default: |
There was a problem hiding this comment.
There's a slight semantic change here which I think wasn't intended. Previously you were OK with "field" not in cell" -- but now for outputs/metadata you add the default. This has caused the notebooks to have "outputs": [] even for text entries that don't usually produce any output.
Probably want to change this to something like if field in cell and cell.get(field) != default? Or have a list of permissible values as well as defaults, like:
for field, default, allowed in fields_defaults:
if cell.get(field) not in allowed:
# ....where you could include None in allowed for "outputs" even while keeping [] as the default.
There was a problem hiding this comment.
I think the "correct" way to format is to have an empty list for code cells, no entry for any other cell. Not sure if the best way to do this is by allowing different defaults by different cell types. What you're suggesting achieves the same result when using a notebook executor because md cells never add the "outputs" key, but it would not roll it back automatically from e.g. the state it is now.
There was a problem hiding this comment.
Came up with a solution for this, it is somewhat different to what we've discussed, let me know if you want to take a look before I merge.
|
@AdamGleave your comment on ValueError is actually on a previous commit -- if you check the current branch, I have this: imitation/ci/check_typeignore.py Lines 81 to 84 in 4620ef6 That's why I marked it as resolved previously |
Ah, that explains it, sorry about that. |
Description
Fixes #532.
Features
np.random.Generatorand require explicit RNG passing to all instances that require random numbers if and only if RNG is being used.# type: ignorethat does not specify the specific rule to be ignored, e.g.# type: ignore[return-value].