save_component carries the component's imports with it - #8
Merged
Merged
Conversation
A component is validated by re-executing its source in an empty namespace, so
anything it reads from another cell has to travel with it. `also` carries a
function and `include` carries a value; an import is the third case and neither
fits it. `also=[Ridge]` tries to inline sklearn's own source and dies on
MultiOutputMixin; `include={'Ridge': Ridge}` writes the class's repr into the
file and produces a SyntaxError.
So a student who writes `from sklearn.linear_model import Ridge` in an import
cell - the normal thing - had their correct model rejected, and was handed two
remedies that both fail. That is the one place the conformance machinery was
brittle in a way a student would experience as us being wrong.
Now the imports are worked out from the names the component actually mentions
and written into the saved file as imports. Name resolution walks up to the
shallowest ancestor module that still exports the object, so the file records
`from sklearn.linear_model import Ridge` rather than the private
`sklearn.linear_model._ridge` path the class reports as its own.
The genuine missing-symbol case is unchanged and still refused: a value or a
helper defined in another cell does not resolve to an importable object, so it
falls through to the existing message naming the symbol.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T1QS8AeUNLQTvUhxZiVSbT
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
A component is validated by re-executing its saved source in an empty namespace with only
numpyandpandasin scope. That is the right design - it is what makes a component survive a cold Colab session - but it means a student who writesand then uses
Ridgeinside their component had their correct model recorded as not conformant.Both remedies the error message suggests fail on an imported class, reproduced before the fix:
also=[Ridge]ValueError: Your model_linear refers to 'MultiOutputMixin'- it tries to inline sklearn's own sourceinclude={'Ridge': Ridge}SyntaxError- it writes the class'sreprinto the fileThis was the one place the conformance machinery was brittle in a way a student experiences as us being wrong, and it fires on the most ordinary thing they can do.
The fix
save_componentworks out the imports from the names the component actually mentions and writes them into the saved file as imports.alsocarries a function,includecarries a value, and an import now travels as what it is.Name resolution walks up to the shallowest ancestor module that still exports the object, so the saved file records the path the student typed:
rather than
sklearn.linear_model._ridge, which is what the class reports as its own module and which the library is free to rename.The name collection is deliberately not scope-aware. It over-collects - a local variable's name lands in the candidate set too - and the caller-namespace lookup is what filters, because a local name does not resolve to an importable object. Precise scoping would be more code for the same result.
What is unchanged
The genuine missing-symbol case is still refused, and there is a test asserting it: a value or helper defined in another cell does not resolve to an importable object, so it falls through to the existing message naming the symbol.
Verification
ruff check src testsclean.🤖 Generated with Claude Code