ludwig icon indicating copy to clipboard operation
ludwig copied to clipboard

render config failed with key error for combiner registry in v0.5.3-v0.5.5

Open qiagu opened this issue 2 years ago • 2 comments

Describe the bug When having explicit combiner in config, ludwig render_config fails with key error as follows

KeyError:
'concat'
  File ".../ludwig/ludwig/utils/registry.py", line 42, in __getitem__
    return self.data.__getitem__(key)
  File ".../ludwig/ludwig/utils/defaults.py", line 206, in merge_with_defaults
    combiner_registry[config[COMBINER][TYPE]].get_schema_cls(), config[COMBINER]
  File ".../ludwig/ludwig/utils/defaults.py", line 226, in render_config
    output_config = merge_with_defaults(config)
  File ".../ludwig/ludwig/utils/defaults.py", line 269, in cli_render_config
    render_config(**vars(args))
  File ".../ludwig/ludwig/cli.py", line 154, in render_config
    defaults.cli_render_config(sys.argv[2:])
  File "...ludwig/ludwig/cli.py", line 64, in __init__
    getattr(self, args.command)()
  File "...ludwig/ludwig/cli.py", line 164, in main
    CLI()
  File ".../ludwig/ludwig/cli.py", line 168, in <module>
    main()
...

To Reproduce run:

ludwig render_config --config_str '{"input_features": [{"name": "A", "type": "number"}], "output_features": [{"name": "B", "type": "category"}], "combiner": {"type": "concat"}}'

Expected behavior key error

Environment (please complete the following information):

  • Python version: 3.8
  • Ludwig version: v0.5.3-0.5.5 v0.5.2 and v0.6.x runs no problem.

qiagu avatar Sep 03 '22 00:09 qiagu

Thanks for raising this issue @qiagu, I'm taking a look. Will revert back with fix once I have one!

arnavgarg1 avatar Sep 03 '22 22:09 arnavgarg1

Looks like it is working on master, but only due to luck. The combiners are getting registered due to an indirect import path, but not because we're being intentional about importing them here:

python -m ludwig.cli render_config --config_str '{"input_features": [{"name": "A", "type": "number"}], "output_features": [{"name": "B", "type": "category"}], "combiner": {"type": "concat"}}'
Traceback (most recent call last):
  File "/Users/tgaddair/.pyenv/versions/3.8.5/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/Users/tgaddair/.pyenv/versions/3.8.5/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/Users/tgaddair/repos/ludwig/ludwig/cli.py", line 176, in <module>
    main()
  File "/Users/tgaddair/repos/ludwig/ludwig/cli.py", line 172, in main
    CLI()
  File "/Users/tgaddair/repos/ludwig/ludwig/cli.py", line 67, in __init__
    getattr(self, args.command)()
  File "/Users/tgaddair/repos/ludwig/ludwig/cli.py", line 160, in render_config
    from ludwig.utils import defaults
  File "/Users/tgaddair/repos/ludwig/ludwig/utils/defaults.py", line 49, in <module>
    from ludwig.data.split import DEFAULT_PROBABILITIES, get_splitter
  File "/Users/tgaddair/repos/ludwig/ludwig/data/split.py", line 23, in <module>
    from ludwig.backend.base import Backend
  File "/Users/tgaddair/repos/ludwig/ludwig/backend/__init__.py", line 20, in <module>
    from ludwig.backend.base import Backend, LocalBackend
  File "/Users/tgaddair/repos/ludwig/ludwig/backend/base.py", line 29, in <module>
    from ludwig.models.base import BaseModel
  File "/Users/tgaddair/repos/ludwig/ludwig/models/base.py", line 9, in <module>
    from ludwig.combiners.combiners import Combiner
  File "/Users/tgaddair/repos/ludwig/ludwig/combiners/combiners.py", line 48, in <module>
    raise RuntimeError("HERE")
RuntimeError: HERE

We should add explicit imports of combiners and other self-registering components here, and add unit tests for the render_config CLI code path (testing the CLI path should help detect this issue, as it will run in a new interpreter).

tgaddair avatar Sep 04 '22 03:09 tgaddair

Hi @qiagu, I'm closing this one for now. If Travis's comment hasn't addressed your issue, please re-open again.

dalianaliu avatar Sep 21 '22 16:09 dalianaliu

Does the Resolved mean that the issue has already been fixed? I don't see any PR relating to the issue though.

qiagu avatar Sep 21 '22 17:09 qiagu

This issue hasn't been fixed yet, but we'll probably roll out a fix as a part of Ludwig 0.6 which comes out soon. Does that work @qiagu?

arnavgarg1 avatar Sep 21 '22 17:09 arnavgarg1

@arnavgarg1 Sure. V0.6 is great.

qiagu avatar Sep 21 '22 17:09 qiagu