fix: keep InstalledPackagesFeatureGroup's output shape on the error path - #1213
Open
dchaudhari7177 wants to merge 1 commit into
Open
fix: keep InstalledPackagesFeatureGroup's output shape on the error path#1213dchaudhari7177 wants to merge 1 commit into
dchaudhari7177 wants to merge 1 commit into
Conversation
calculate_feature returned {ClassName: [packages]} on success but
{"error": message} on CalledProcessError -- a different key, and a bare string
where the success path returns a list. The class docstring's Output Format
section promises the single ClassName column.
The consequence runs the wrong way: a caller hits a KeyError or a malformed
frame exactly when pip freeze has failed and it is trying to read why. The
error path was the thing making errors hard to diagnose.
Both paths now return {ClassName: [message]}.
The Implementation Details bullet ("On error, returns error message
dictionary") described the old behaviour and would have contradicted the code,
so it is updated with it.
The failure branch had no coverage. The new test patches subprocess.run to
raise and asserts set(result) == {column} rather than `column in result`: an
`in` check still passes if a stray "error" key comes back alongside the right
one, which is the regression worth catching.
Confirmed the test fails against the old return, and passes with the fix.
Closes mloda-ai#1206
Contributor
|
@dchaudhari7177 Please address the security findings in the CI. |
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
2 participants
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.
Closes #1206
calculate_featurereturned{cls.get_class_name(): [packages]}on success but{"error": error_message}onCalledProcessError— a different key, and a bare string where the success path returns a list. The class's own Output Format docstring promises a single column namedInstalledPackagesFeatureGroup.The consequence is the wrong way round: a caller gets a
KeyErroror a malformed frame precisely whenpip freezehas failed and it needs to read the error message. The failure path was the one making failures hard to diagnose.Now
{cls.get_class_name(): [error_message]}— same key, same list-of-one shape, on both paths.I also corrected the docstring's Implementation Details bullet, which said "On error, returns error message dictionary". That line described the old behaviour and would have contradicted the code after this change.
Test
The failure branch had no coverage at all. The new test patches
subprocess.runto raiseCalledProcessErrorand asserts:set(result) == {column}rather thancolumn in resulton purpose — anincheck would still pass if a stray"error"key came back alongside the right one, which is exactly the regression worth catching.Verified it fails against the old code:
With the fix: 3 passed.
ruff format/ruff checkclean.mypy --stricton the changed file reports 6 errors in 4 other files (missing pandas stubs in my local env) — byte-identical output onupstream/mainwith my changes stashed, so nothing new from this PR.