Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CU-8694py1jr fix old config load with reg json #449

Merged
merged 7 commits into from
Jun 3, 2024

Conversation

mart-r
Copy link
Collaborator

@mart-r mart-r commented May 31, 2024

The last PR for (almost) primitive config (#425) had a few issues:

  • It didn't correctly identify all old style configs (e.g ConfigMetaCAT)
  • It didn't correctly apply options loaded from old style configs (even for regular config)

This PR fixes the above.
It also adds tests for old types of config (regular, MetaCAT, TNER, and RelCAT).
For each, it makes sure that they're classified as old style, and that a value changed within them is correctly identified as having been changed / loaded off disk.

@tomolopolis
Copy link
Member

@mart-r mart-r changed the title CU-8694py1jr fix other configs with reg json CU-8694py1jr fix old config load with reg json May 31, 2024
@shubham-s-agarwal
Copy link
Collaborator

all looks good, tested on the metacat models from KCH, works fine now!

@@ -15,9 +15,24 @@ def weighted_average_function(self) -> Callable[[float], int]:


def is_old_type_config_dict(d: dict) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

This assumes all configs were created post the pydantic changes to the config right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that would be true since before that it would have been saved as a raw dict.

@tomolopolis tomolopolis merged commit 03c6881 into master Jun 3, 2024
8 checks passed
@mart-r mart-r deleted the CU-8694py1jr-fix-other-configs-with-reg-json branch August 12, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants