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-8694fwyje pre load config(s) #447

Closed
wants to merge 27 commits into from
Closed

Conversation

mart-r
Copy link
Collaborator

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

EDIT:
This PR has an alternative approach in #473 .

Separate config entries that only (truly) take effect if set before a model is loaded or initialised.

Some of these config entries are used elsewhere in the codebase as well. But most of the time it looks (at least to me) like their main use is at init/load time.

EDIT: This will also add translation layers to the configs for legacy mappings when loading the various configs (main, MetaCAT, TNER, RelCAT).

Also removed some of the unused config entries in RelCAT config.

This affects

  • Normal medcat config
  • Transformers NER config
  • MetaCAT config
  • RelCAT config

NOTE:
I'm open to discussions on which config entries should stay where they were as well as which ones shouldn't be removed from RelCAT config.

PS:
This might create issues downstream (i.e in tutorials and WWC). So if/when this PR will be approved, I'll look into creating PRs for those to go along with this one.

PPS:
The main config's linking.subsample_after option is also unused. However, there is commented code for it to be used in context_based_linker.Linker._train so I left it in for now.

@tomolopolis
Copy link
Member

@mart-r
Copy link
Collaborator Author

mart-r commented May 31, 2024

Note: still need to add translation layers (along with tests) that allow loading and converting old format configs.

@mart-r
Copy link
Collaborator Author

mart-r commented May 31, 2024

This is a WIP. Didn't manage to get rid of all the issues.

Best I can tell, before (i.e when actions was successful, e.g after 49d25ba) the config(s) loaded off disk weren't really being applied due to having no remapping. As such, some specific tests weren't being run as expected since they used default config instead.

This may also have something to do with the other config-related PR #449 since that might mean that some things aren't saved and/or loaded correctly on this branch.

In any case, ran out of time.
So I'll pick back up once I'm back, so on the 10th of June.

Copy link
Collaborator

@shubham-s-agarwal shubham-s-agarwal left a comment

Choose a reason for hiding this comment

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

I've checked this for MetaCAT, looks good to me!

@vladd-bit
Copy link
Member

RelCAT (old version) should be good too :)

@mart-r
Copy link
Collaborator Author

mart-r commented Aug 12, 2024

Decided to go with #473 instead

@mart-r mart-r closed this Aug 12, 2024
@mart-r mart-r deleted the CU-8694fwyje-pre-load-config branch November 18, 2024 16:23
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.

4 participants