feat: Updated BaseConfig class for non primitive fields #1684
Conversation
There was a problem hiding this comment.
Thank you @abhiramvad for the contribution! Given this changes a very sensitive class, would you be able to add unit tests to validate the logic ?
|
Sure, I need some time to add them. Will do that soon. |
I made the changes requested previously in comments. I have added few baseline tests, checking the test_config.py . Hope they align with what is needed. Kindly provide me comments on those. I'll update them soon. |
|
Hi @abhiramvad , could you please fix the precommit errors? You can verify it passes locally by running |
|
Thank you @abhiramvad for the updates! I think this is very close to the finish line. It looks like some of the new tests are failing? |
|
can we run the workflow again please? |
|
@abhiramvad could you fix the test error? You can run the checks locally with |
|
@wizeng23 i fixed these errors. Here is a precommit check screenshot from my machine The tests work on my machine, and i also ran the CPU tests workflow on my fork. Lets hope it doesn't break here... https://github.com/abhiramvad/oumi/actions/runs/18120697923 |
|
@wizeng23 can you review please? |
| def _is_primitive_type(value: Any) -> bool: | ||
| """Check if a value is of a primitive type that OmegaConf can handle.""" | ||
| return ( | ||
| isinstance(value, (str, int, float, bool, bytes)) |
There was a problem hiding this comment.
nit: could you re-use _PRIMITIVE_TYPES here to reduce duplication? I believe you still need the separate checks for Path and Enum though.
return (
type(value) in _PRIMITIVE_TYPES
or isinstance(value, Path)
or isinstance(value, Enum)
)
| lora_alpha=self.lora_alpha, | ||
| lora_dropout=self.lora_dropout, | ||
| target_modules=target_modules, | ||
| target_parameters=self.lora_target_parameters, |
There was a problem hiding this comment.
Can you add this line back?
| ) | ||
|
|
||
|
|
||
| def _handle_non_primitives(config: Any, removed_paths, path: str = "") -> Any: |
There was a problem hiding this comment.
Can you add type to removed_paths?
| processed_value = _handle_non_primitives( | ||
| field_value, removed_paths, current_path | ||
| ) | ||
| if processed_value is not None: |
There was a problem hiding this comment.
IMO if we redact a value, we should keep the key and have the value be None. You did something similar up in line 79.
| field_value is not None | ||
| ): # Only track removal if original value was not None | ||
| removed_paths.add(current_path) | ||
| return result if result else None |
There was a problem hiding this comment.
Could you always return result here and not None? I'd rather have empty dict than a None value to properly reflect the object.
3 participants
Description
This is a PR for the issue 1628. The
to_yamldoes not warn when trying to save non primitive data types (such as functions). This leads to errors while loading from saved yaml files.I have added a check to gracefully check for non primitive types recursively. If present, they are removed from the config. The removed types and their paths are specified to the user via logs.
Related issues
Fixes # (issue)
#1628
Before submitting
Reviewers
At least one review from a member of
oumi-ai/oumi-staffis required.