Skip to content

Conversation

abhiramvad
Copy link
Contributor

Description

This is a PR for the issue 1628. The to_yaml does 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

  • This PR only changes documentation. (You can ignore the following checks in that case)
  • Did you read the contributor guideline Pull Request guidelines?
  • Did you link the issue(s) related to this PR in the section above?
  • Did you add / update tests where needed?

Reviewers

At least one review from a member of oumi-ai/oumi-staff is required.

Copy link
Contributor

@wizeng23 wizeng23 left a comment

Choose a reason for hiding this comment

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

Thank you for creating this PR! I've left a couple comments. Could you also confirm that you've tested this PR, and that it works for non-primitives, such as functions?

Copy link
Contributor

@oelachqar oelachqar left a comment

Choose a reason for hiding this comment

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

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 ?

@abhiramvad
Copy link
Contributor Author

Sure, I need some time to add them. Will do that soon.

@abhiramvad
Copy link
Contributor Author

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 ?

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.

@wizeng23
Copy link
Contributor

Hi @abhiramvad , could you please fix the precommit errors? You can verify it passes locally by running pre-commit run --all-files --show-diff-on-failure.

@abhiramvad abhiramvad requested review from oelachqar and wizeng23 July 20, 2025 19:52
@oelachqar
Copy link
Contributor

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?

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