fix(core): declare the dataset fields that core paradigms read - #891
Open
akshita317 wants to merge 1 commit into
Open
fix(core): declare the dataset fields that core paradigms read#891akshita317 wants to merge 1 commit into
akshita317 wants to merge 1 commit into
Conversation
Dataset._parse_config assigns only keys that already exist on the instance:
for attr, value in config.items():
if attr in self.__dict__:
self.__dict__[attr] = value
Four fields are read off the Dataset instance by core paradigm code but are
not declared in Dataset.__init__, so any value a testenv.yaml supplies for
them is silently discarded and the later attribute access raises:
val_url core/.../singletask_learning_tta.py:77
known_dataset_url core/.../singletask_learning_active_boost.py:61
unknown_dataset_url core/.../singletask_learning_active_boost.py:62
image_folder_url core/.../singletask_learning_active_boost.py:49,63
core/.../singletask_learning_tta.py:46,76
Reproduced against the real classes on main, using the example config
unmodified:
dataset.image_folder_url -> AttributeError: 'Dataset' object has no
attribute 'image_folder_url'
dataset.known_dataset_url -> AttributeError
dataset.unknown_dataset_url -> AttributeError
git log -S image_folder_url over core/testenvmanager/dataset/dataset.py
returns nothing, so these were never declared; the paradigms that read them
and the example that configures them arrived together in 7111198 without the
corresponding registration.
The change is four attribute declarations defaulting to the empty string,
matching the surrounding fields. It is additive: examples that do not set
these keys keep the same empty-string default and behave exactly as before.
Verified after the change:
- the three attribute reads above resolve to the configured paths
- Dataset constructs for all 48 examples/**/testenv*.yaml (48 ok, 0 failed)
- Rank constructs for all 43 rank blocks (43 ok, 0 failed)
- pylint --max-positional-arguments=10 core/ stays at 10.00/10
This does not by itself make examples/yaoba run; the paradigm dispatch work in
kubeedge#824 is also required, and the two are independent. It does make
the path corrections in kubeedge#796 and kubeedge#824 take effect,
which they currently cannot.
Signed-off-by: Akshita <110122283+akshita317@users.noreply.github.com>
Collaborator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akshita317 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Author
|
/assign @MooreZheng Small Core change (10 lines, additive) unblocking the yaoba path fixes in #796 and #824. Verified against all 48 example testenv configs and all 43 rank blocks with no regression; details in the description. |
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.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Dataset._parse_config(core/testenvmanager/dataset/dataset.py:71-73) assigns only keys that already exist on the instance:Four fields are read off the
Datasetinstance by core paradigm code, but are not declared inDataset.__init__. Any value atestenv.yamlsupplies for them is therefore discarded without warning, and the later attribute access raises:val_urlcore/.../singletask_learning_tta.py:77known_dataset_urlcore/.../singletask_learning_active_boost.py:61unknown_dataset_urlcore/.../singletask_learning_active_boost.py:62image_folder_urlcore/.../singletask_learning_active_boost.py:49,63andsingletask_learning_tta.py:46,76Reproduced on
mainagainst the real classes, usingexamples/yaoba/singletask_learning_boost/testenv/testenv.yamlunmodified:git log --all -S image_folder_url -- core/testenvmanager/dataset/dataset.pyreturns nothing, so these were never declared. The paradigms that read them and the example that configures them both arrived in7111198, without the matching registration.The change is four attribute declarations defaulting to
"", consistent with the surrounding fields. It is purely additive — examples that do not set these keys keep the same empty-string default and behave exactly as before.Verification
Datasetconstructed for everyexamples/**/testenv*.yamlRankconstructed for everyrank:blockpylint --max-positional-arguments=10 core/mainNot verified: a full
ianvs -frun ofexamples/yaoba. That example pinsmmcv-full 1.7.1andmmdet 2.28.2and needs the YaoBa image corpus, which has no public download, so I could not execute it end to end and am not claiming it passes.Which issue(s) this PR fixes:
Related to #742 and #823.
Special notes for your reviewer:
This does not on its own make
examples/yaobarun — the paradigm dispatch registration in #824 (@31groot) is also needed, and the two changes are independent and do not conflict.What it does do is make the path corrections in #796 (@hibaa23) and in #824 effective. Both correct
image_folder_urlto a repo-relative path, and at present that value cannot reach core because the key is dropped here. @hibaa23 confirmed on #796 that they are happy to defer the config-line edit to whichever PR carries the core registration; this PR deliberately touches no example file, so it should not conflict with either.@Nikhil-VS1811, who is assigned to #742, and I split the work on that issue: this registration is the piece I picked up, and they are looking at the broader silent-drop pattern across the other example families.
Scope is deliberately narrow. Auditing all 48
examples/**/testenv*.yamlagainst the fieldsDataset.__init__declares, 12 configs declare at least one dropped key, but cross-checking everyself.dataset.<attr>read incore/against those declarations closes the set at exactly these four:The dropped keys in the other 10 configs (
cifar100,federated-llm,PIPL,industrialEI) have no consumer anywhere incore/orexamples/, so they are dead config or human-readable metadata rather than this defect, and registering them would be speculative. Full audit and script in #742.Does this PR introduce a user-facing change?: