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

add pathlib.Path support #873

Merged
merged 6 commits into from
May 12, 2022
Merged

add pathlib.Path support #873

merged 6 commits into from
May 12, 2022

Conversation

gwenzek
Copy link
Contributor

@gwenzek gwenzek commented Mar 7, 2022

Add support for Path in Omegaconf.

Path type will never be automatically inferred when loading from YAML, so we will continue creating StringNode by default.
But if you use structured or _promote with a dataclass using Path some of StringNode can be converted to PathNode eg:

https://github.com/gwenzek/omegaconf/blob/1df8a05f9f740b7085fb65fe7a4d8b1e034f2ea6/tests/structured_conf/test_structured_config.py#L819-L833

This change is backward compatible, because code that currently use dataclass with Path fields currently see errors when running structured.

This should resolve #97

cc: @odelalleau @Mortimerp9

@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2022

This pull request introduces 1 alert when merging 1df8a05 into 5e73ee6 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@Jasha10
Copy link
Collaborator

Jasha10 commented Mar 7, 2022

Closes #97.

@Jasha10
Copy link
Collaborator

Jasha10 commented Mar 7, 2022

Thanks very much for taking the time to create this PR, @gwenzek.
At the moment, I can't commit to a timeline for merging this, as the OmegaConf team currently has it's hands full.
Edit: we will review this shortly.

Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

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

>>> OmegaConf.create({"foo": Path("bar")})
Traceback (most recent call last):
...
omegaconf.errors.UnsupportedValueType: Value 'PosixPath' is not a supported primitive type
    full_key: foo
    object_type=dict

I think we should support the above use-case. This would probably require updating the is_primitive_type function

@gwenzek
Copy link
Contributor Author

gwenzek commented Mar 9, 2022

good point, I've added a test case for this,
and indeed I had to modify is_primitive to allow this.

Note that I've extrcted the list of base types to a constant for cosmetic reasons.

@gwenzek gwenzek requested a review from Jasha10 March 9, 2022 16:36
@Jasha10
Copy link
Collaborator

Jasha10 commented Mar 9, 2022

Thanks @gwenzek. The implementation looks good to me.
I am planning to push some commits to this PR branch adding more tests.

@Jasha10 Jasha10 self-assigned this Mar 9, 2022
@gwenzek
Copy link
Contributor Author

gwenzek commented Mar 14, 2022

I rebased to resolve the conflict with the new bytes node.

omegaconf/_utils.py Show resolved Hide resolved
@Jasha10 Jasha10 added this to the OmegaConf 2.2 milestone Apr 14, 2022
@gwenzek gwenzek requested a review from Jasha10 April 14, 2022 12:10
@Jasha10
Copy link
Collaborator

Jasha10 commented May 10, 2022

Demo of the API:

>>> from omegaconf import OmegaConf; from pathlib import Path
>>> OmegaConf.create({"foo": Path("hello.txt")})
{'foo': PosixPath('hello.txt')}

pathlib.Path can be used as a type annotation with structured configs too.
Paths are preserved through a round-trip to yaml:

>>> cfg = OmegaConf.create({"foo": Path("hello.txt")})
>>> print(OmegaConf.to_yaml(cfg))
foo: !!python/object/apply:pathlib.PosixPath
- hello.txt

>>> OmegaConf.create(OmegaConf.to_yaml(cfg))
{'foo': PosixPath('hello.txt')}

Notes:

  • I've disabled automatic conversion from Path to str. This means assigning
    Path("foo.txt") to MyStructuredConf.string_field fails with a
    ValidationError. Conversion from string to path is implemented (assigning the
    string "foo.txt" to MyStructuredConf.path_field succeeds).
  • Using the yaml tag !!python/object/apply:... means that the output of OmegaConf.to_yaml can be deserialized using vanilla pyyaml (i.e. without using OmegaConf):
    import yaml
    yaml_document = OmegaConf.to_yaml(OmegaConf.create({"foo": Path("hello.txt")}))
    assert OmegaConf.create(yaml_document) == yaml.unsafe_load(yaml_document)

cc @rsokl, @odelalleau

@Jasha10 Jasha10 requested review from jieru-hu and pixelb May 10, 2022 15:51
Copy link
Collaborator

@jieru-hu jieru-hu left a comment

Choose a reason for hiding this comment

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

looks great!

@gwenzek
Copy link
Contributor Author

gwenzek commented May 11, 2022

Thanks a lot @Jasha10 for adding the necessary tests and a updating the documentation.

@Jasha10 Jasha10 merged commit 310e603 into omry:master May 12, 2022
@Mortimerp9
Copy link

Awesome! Thanks for getting this in Hydra!

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.

Add support for pathlib.Path type
4 participants