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

Implicit Path() to str conversion fails in 2.2.1 #934

Closed
4 tasks done
titu1994 opened this issue May 18, 2022 · 4 comments · Fixed by #941
Closed
4 tasks done

Implicit Path() to str conversion fails in 2.2.1 #934

titu1994 opened this issue May 18, 2022 · 4 comments · Fixed by #941
Labels
bug Something isn't working

Comments

@titu1994
Copy link

titu1994 commented May 18, 2022

Describe the bug

When assigning a value to a structured dataclass, attempting to convert a Path() object implicitly to str fails -

Traceback (most recent call last):
  File "tests/core_ptl/check_for_ranks.py", line 154, in <module>
    run_checks()
  File "tests/core_ptl/check_for_ranks.py", line 144, in run_checks
    trainer = instantiate_multinode_ddp_if_possible()
  File "tests/core_ptl/check_for_ranks.py", line 83, in instantiate_multinode_ddp_if_possible
    exp_manager(trainer, cfg=OmegaConf.structured(exp_manager_cfg))
  File "/opt/conda/lib/python3.8/site-packages/nemo/utils/exp_manager.py", line 332, in exp_manager
    configure_checkpointing(
  File "/opt/conda/lib/python3.8/site-packages/nemo/utils/exp_manager.py", line 871, in configure_checkpointing
    params.dirpath = Path(log_dir / 'checkpoints')
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/dictconfig.py", line 340, in __setattr__
    self._format_and_raise(key=key, value=value, cause=e)
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/base.py", line 231, in _format_and_raise
    format_and_raise(
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/_utils.py", line 873, in format_and_raise
    _raise(ex, cause)
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/_utils.py", line 771, in _raise
    raise ex.with_traceback(sys.exc_info()[2])  # set env var OC_CAUSE=1 for full trace
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/dictconfig.py", line 336, in __setattr__
    self.__set_impl(key, value)
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/dictconfig.py", line 320, in __set_impl
    self._set_item_impl(key, value)
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/basecontainer.py", line 602, in _set_item_impl
    self.__dict__["_content"][key]._set_value(value)
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/nodes.py", line 46, in _set_value
    self._val = self.validate_and_convert(value)
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/nodes.py", line 76, in validate_and_convert
    return self._validate_and_convert_impl(value)
  File "/opt/conda/lib/python3.8/site-packages/omegaconf/nodes.py", line 194, in _validate_and_convert_impl
    raise ValidationError("Cannot convert '$VALUE_TYPE' to string: '$VALUE'")
omegaconf.errors.ValidationError: Cannot convert 'pathlib.PosixPath' to string: 'ddp_check/default/version_0/checkpoints'
    full_key: checkpoint_callback_params.dirpath
    reference_type=Optional[CallbackParams]
    object_type=CallbackParams 

To Reproduce

Python file :

from dataclasses import dataclass
from typing import Any, Dict, List, Optional, Union
from omegaconf import OmegaConf
from pathlib import Path
import omegaconf

@dataclass
class CallbackParams:
    dirpath: Optional[str] = None  # If None, exp_manager will attempt to handle the filepath

if __name__ == '__main__':
    print("OmegaConf version :", omegaconf.version.__version__)

    cfg = OmegaConf.structured(CallbackParams)  # type: CallbackParams
    logdir = Path.cwd()
    cfg.dirpath = logdir / Path('logs')

Output :

/home/smajumdar/miniconda3/envs/NeMo/bin/python /home/smajumdar/PycharmProjects/nemo-eval/tmp/temp.py
OmegaConf version : 2.2.1

Traceback (most recent call last):
  File "/home/smajumdar/PycharmProjects/nemo-eval/tmp/temp.py", line 18, in <module>
    cfg.dirpath = logdir / Path('logs')
  File "/home/smajumdar/miniconda3/envs/NeMo/lib/python3.8/site-packages/omegaconf/dictconfig.py", line 340, in __setattr__
    self._format_and_raise(key=key, value=value, cause=e)
  File "/home/smajumdar/miniconda3/envs/NeMo/lib/python3.8/site-packages/omegaconf/base.py", line 231, in _format_and_raise
    format_and_raise(
  File "/home/smajumdar/miniconda3/envs/NeMo/lib/python3.8/site-packages/omegaconf/_utils.py", line 873, in format_and_raise
    _raise(ex, cause)
  File "/home/smajumdar/miniconda3/envs/NeMo/lib/python3.8/site-packages/omegaconf/_utils.py", line 771, in _raise
    raise ex.with_traceback(sys.exc_info()[2])  # set env var OC_CAUSE=1 for full trace
  File "/home/smajumdar/miniconda3/envs/NeMo/lib/python3.8/site-packages/omegaconf/dictconfig.py", line 336, in __setattr__
    self.__set_impl(key, value)
  File "/home/smajumdar/miniconda3/envs/NeMo/lib/python3.8/site-packages/omegaconf/dictconfig.py", line 320, in __set_impl
    self._set_item_impl(key, value)
  File "/home/smajumdar/miniconda3/envs/NeMo/lib/python3.8/site-packages/omegaconf/basecontainer.py", line 602, in _set_item_impl
    self.__dict__["_content"][key]._set_value(value)
  File "/home/smajumdar/miniconda3/envs/NeMo/lib/python3.8/site-packages/omegaconf/nodes.py", line 46, in _set_value
    self._val = self.validate_and_convert(value)
  File "/home/smajumdar/miniconda3/envs/NeMo/lib/python3.8/site-packages/omegaconf/nodes.py", line 76, in validate_and_convert
    return self._validate_and_convert_impl(value)
  File "/home/smajumdar/miniconda3/envs/NeMo/lib/python3.8/site-packages/omegaconf/nodes.py", line 194, in _validate_and_convert_impl
    raise ValidationError("Cannot convert '$VALUE_TYPE' to string: '$VALUE'")
omegaconf.errors.ValidationError: Cannot convert 'pathlib.PosixPath' to string: '/home/smajumdar/PycharmProjects/nemo-eval/tmp/logs'
    full_key: dirpath
    object_type=CallbackParams

Process finished with exit code 1

Expected behavior

/home/smajumdar/miniconda3/envs/NeMo/bin/python /home/smajumdar/PycharmProjects/nemo-eval/tmp/temp.py
OmegaConf version : 2.1.2

Process finished with exit code 0

Additional context

  • OmegaConf version: 2.2.1
  • Python version: 3.8
  • Operating system: Linux, Ubuntu 20.04
  • Please provide a minimal repro
@titu1994 titu1994 added the bug Something isn't working label May 18, 2022
@titu1994 titu1994 changed the title Path() to str conversion fails in 2.2.1 Implicit Path() to str conversion fails in 2.2.1 May 18, 2022
@Jasha10
Copy link
Collaborator

Jasha10 commented May 18, 2022

Hi @titu1994,
OmegaConf 2.2.1 supports pathlib.Path natively. In supporting pathlib.Path, we turned off implicit conversion from Path to str.
I didn't realize this would be a breaking change as I hadn't considered that users might rely on the implicit conversion when assigning Path instances to string-typed structured config nodes. I'll bring this up with the Hydra team.

It's now possible to use Path as a type hint in structured configs:

@dataclass
class CallbackParams2:
    dirpath: Optional[Path] = None

Would that work for your use-case? Another workaround could be to convert to a string explicitly:

    cfg.dirpath = str(logdir / Path('logs'))

@titu1994
Copy link
Author

titu1994 commented May 18, 2022

While sure explicitly casting dataclasses to Path instead of str would work, in our case we have some 150+ configs (public, internal, external users etc), and over 50+ dataclasses which somewhere or other depend on this automatic conversion. It would be nearly impossible to safely upgrade all of them, and intractable to do it even for just the public ones.

It would be more useful to support implicit casting - because Python in general makes no major distinction between the two for files and treats Path() as a string implicitly as needed.

More interestingly, semantically we have the dataclass argument as type str - and path can be casted to str via str(path) so the auto conversion should work out of the box even if Path() is passed during assignment.

@pixelb
Copy link
Collaborator

pixelb commented May 19, 2022

Implicit Path() to string conversion seems common enough that we should maintain support.

@titu1994
Copy link
Author

Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants