test(serialization): make the serialization suite runnable and add it to CI - #696
test(serialization): make the serialization suite runnable and add it to CI#696Hotragn wants to merge 1 commit into
Conversation
… to CI
The serialization tests are not in the CI list, and two of the four files had
stopped working without anyone noticing.
`tests/test_whitelist_serialization.py` did not collect at all:
ImportError: cannot import name 'SERVER_MODULES_WHITELIST' from
'nnsight.intervention.serialization'
`SERVER_MODULES_WHITELIST` and `_is_whitelisted_module` were imported at
module scope, so their removal took all 12 tests in the file down -- including
nine that test source serialization and still pass. Seven tests in
`test_serialization_edge_cases.py` failed the same way on
`PicklingProhibitedError`.
Both are now resolved per-test and skip with a stated reason instead of
erroring at import, so the rest of each file runs and these come back on their
own if the APIs return.
Four more tests were order-dependent, and passed only by accident.
`test_no_register_needed`, `test_serialization_includes_module_functions` and
the two `*_serialized_by_source` tests all assert `b"def normalize" in
dumps(normalize)`. Serializing by source is opt-in: unregistered,
`dumps(normalize)` is a 47-byte GLOBAL with no source; registered, it is 887
bytes with the definition. `register` mutates a process-global set in
cloudpickle, and every trace calls `get_local_env()`, which registers local
modules -- so these four passed in a whole-file run and every one of them
failed when run alone. They now use a fixture that registers and unregisters
around each test, and a new `unregistered_mymethods` fixture lifts the entries
back out so the by-reference half of the contract can be asserted wherever it
runs. Every test in the file now passes (or skips) in its own process.
`test_no_register_needed` was also asserting the opposite of current
behaviour: its premise, that `register()` is unnecessary, held under the
whitelist design that has since been removed. It is replaced by
`test_register_switches_to_serialization_by_value` (registration is what makes
source travel with the job) and `test_unregistered_module_is_serialized_by_
reference` (without it, the server gets a GLOBAL it cannot import).
`serialization.py` still carried a `# SECURITY CHECK: Check function globals
for prohibited modules/functions` comment and a `captured_globals` local that
nothing read -- the check itself is gone. Left as-is it reads as though input
is being validated there. Replaced with a note saying the check is absent,
where it would go, and that its tests are already written.
CI now runs all four serialization files: 323 tests where the list previously
covered 248.
Note: the run will stay red until ndif-team#693 lands --
`test_lm.py::TestGradients::test_backward_with_multiple_invokers` is already
failing on `dev` and is unrelated to this change.
|
Hey @Hotragn ! Thank you so much for the recent PRs! Right now im actively developing off the 0.8 branch here: https://github.com/ndif-team/nnsight/tree/0.8 |
|
Thanks — moved over to Checked each piece against
So the whole PR is redundant on Two things did come out of running the
Also updated #695 — most of what I raised there is resolved on |
|
Will do — everything is on I re-checked all four PRs against
Two are still live, both verified against
#699 will show the two One note in case it's useful: |
Summary
The four serialization test files are not in the CI list, and two of them had stopped working entirely without anyone noticing. This makes the suite runnable, removes the order-dependence in it, corrects a comment in
serialization.pythat claims a check which isn't there, and adds all four files to CI.Found while checking which test files CI does not cover.
What was broken
1.
test_whitelist_serialization.pydid not collect at all.SERVER_MODULES_WHITELIST/_is_whitelisted_modulewere imported at module scope, so their removal took all 12 tests in the file down — including the nine that test source serialization and still pass. Seven tests intest_serialization_edge_cases.pyfailed the same way onPicklingProhibitedError.Both are now resolved per-test and skip with a stated reason rather than erroring at import. The rest of each file runs, and these come back automatically if the APIs return — which felt better than deleting the maintainers' intent.
2. Four tests were order-dependent and passed by accident.
test_no_register_needed,test_serialization_includes_module_functions, and the two*_serialized_by_sourcetests all assertb"def normalize" in dumps(normalize). Serializing by source is opt-in:registermutates a process-global set in cloudpickle (_PICKLE_BY_VALUE_MODULES), and every trace callsget_local_env(), which registers local modules. So these four passed in a whole-file run — and every one of them failed when run alone:They now use a fixture that registers and unregisters around each test. A second fixture lifts the entries back out of the global set so the by-reference half of the contract can be asserted wherever in the file it runs. Every test in the file now passes (or skips) in its own process.
3.
test_no_register_neededasserted the opposite of current behaviour.Its premise — that
register()is unnecessary — was true under the whitelist design that has since been removed. Without registration,dumpsemits a GLOBAL the server cannot import, which is theModuleNotFoundErrorthatdocs/remote/register-local-modules.mdexists to prevent. Replaced with two tests that pin the real contract in both directions:test_register_switches_to_serialization_by_value— registration is what makes source travel with the jobtest_unregistered_module_is_serialized_by_reference— without it, the payload is a bareGLOBAL mymethods.stateful normalize4.
serialization.pyclaimed a security check it no longer performs.captured_globalsis assigned and never read — it is the only remaining trace of the lint-time blacklist added in ccb1702. Left as-is, this reads to anyone auditing the remote path as though input is being validated there. Replaced with a note stating the check is absent, where it would go, and that its tests are already written.I have not restored the blacklist itself — see #695 for that question. It would start rejecting user code that references
pathlib,io,globand friends, which is a policy call rather than something to slip into a test-coverage PR.Result
75 previously-unrun tests, covering the path that ships user code to NDIF.
Note
The CI run will stay red until #693 lands.
test_lm.py::TestGradients::test_backward_with_multiple_invokersis already failing ondevandmainand is unrelated to this change. Happy to rebase this on top of #693, or to drop the workflow change into a follow-up if you'd rather land the test repairs first.Run with
transformers5.15.1 /torch2.9.1 /cloudpickle3.1.2 on CPU.