Skip to content

fix: Config save cleanup#113

Merged
andrea-fasoli merged 6 commits intofoundation-model-stack:mainfrom
BrandonGroth:config_save_cleanup
May 10, 2025
Merged

fix: Config save cleanup#113
andrea-fasoli merged 6 commits intofoundation-model-stack:mainfrom
BrandonGroth:config_save_cleanup

Conversation

@BrandonGroth
Copy link
Copy Markdown
Collaborator

@BrandonGroth BrandonGroth commented May 8, 2025

Description of the change

This PR fixes issues found with the config_save feature added in #103:

  • Fixed qconfig_save in qmodel_prep using recipe=qcfg.json instead of fname. This would cause an error re-running qconfig_save as the JSON was not a list.
  • Added 4 tests to ensure the above bug has coverage as well as ensuring JSON save/load is being checked properly.
  • Guards the qconfig_save/load from opening non-dictionary/list json files.
  • Added smoothq prefix to qcfg variables.
  • Added smoothq variables to the default config.

Was the PR tested

  • I have ensured all unit tests pass

Signed-off-by: Brandon Groth <brandon.m.groth@gmail.com>
…, and added smoothq vars to default config

Signed-off-by: Brandon Groth <brandon.m.groth@gmail.com>
Signed-off-by: Brandon Groth <brandon.m.groth@gmail.com>
Signed-off-by: Brandon Groth <brandon.m.groth@gmail.com>
Signed-off-by: Brandon Groth <brandon.m.groth@gmail.com>
Signed-off-by: Brandon Groth <brandon.m.groth@gmail.com>
@BrandonGroth BrandonGroth force-pushed the config_save_cleanup branch from 3a5b01b to 02535d1 Compare May 9, 2025 18:01
Copy link
Copy Markdown
Collaborator

@tharapalanivel tharapalanivel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!


save_json(config_list, file_path="qcfg.json")

with pytest.raises(ValueError):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: tiny comment is to actually check that it fails Quantized config={fname} is not a dictionary but optional

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am missing something, but I setup qconfig_load to only accept a dict datatype from the json.load, or it errors. So this check should force an error.

Copy link
Copy Markdown
Collaborator

@andrea-fasoli andrea-fasoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, ready to merge

@andrea-fasoli andrea-fasoli merged commit 199e3d1 into foundation-model-stack:main May 10, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants