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

ValueError when target name contains a . #25

Closed
reiyw opened this issue Jun 2, 2022 · 1 comment
Closed

ValueError when target name contains a . #25

reiyw opened this issue Jun 2, 2022 · 1 comment
Assignees

Comments

@reiyw
Copy link

reiyw commented Jun 2, 2022

The BazelContainer documentation uses image.tar as an example target name, which actually returns a ValueError.

Traceback
Traceback (most recent call last):
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/.venv/bin/xmanager", line 33, in <module>
    sys.exit(load_entry_point('xmanager', 'console_scripts', 'xmanager')())
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/xmanager/cli/cli.py", line 65, in entrypoint
    app.run(main)
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/.venv/lib/python3.9/site-packages/absl/app.py", line 312, in run
    _run_main(main, args)
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/.venv/lib/python3.9/site-packages/absl/app.py", line 258, in _run_main
    sys.exit(main(argv))
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/xmanager/cli/cli.py", line 41, in main
    app.run(m.main, argv=argv)
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/.venv/lib/python3.9/site-packages/absl/app.py", line 312, in run
    _run_main(main, args)
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/.venv/lib/python3.9/site-packages/absl/app.py", line 258, in _run_main
    sys.exit(main(argv))
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/launcher.py", line 7, in main
    [executable] = experiment.package(
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/xmanager/xm/core.py", line 636, in package
    return cls._async_packager.package(packageables)
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/xmanager/xm/async_packager.py", line 114, in package
    executables = self._package_batch(packageables)
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/xmanager/xm_local/packaging/router.py", line 112, in package
    bazel_kinds = bazel_service.fetch_kinds(bazel_labels)
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/xmanager/xm_local/packaging/bazel_tools.py", line 186, in fetch_kinds
    labels = [_assemble_label(_lex_label(label)) for label in labels]
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/xmanager/xm_local/packaging/bazel_tools.py", line 186, in <listcomp>
    labels = [_assemble_label(_lex_label(label)) for label in labels]
  File "/Users/ryo.takahashi/ghq/github.com/deepmind/xmanager/xmanager/xm_local/packaging/bazel_tools.py", line 160, in _lex_label
    raise ValueError(f'{label} is not an absolute Bazel label')
ValueError: //path/to/target:label.tar is not an absolute Bazel label

This error is caused by _LABEL_LEXER. This regex does not allow the inclusion of a single . which is not an expansion, so the label name in the example will not match.
https://github.com/deepmind/xmanager/blob/18652570332e284a6b2c184e6ab943ca56f6a11a/xmanager/xm_local/packaging/bazel_tools.py#L149-L152

The immediate solution that comes to mind is to allow containing . in the regex, then look for a consecutive . in post-processing like the following:

diff --git a/xmanager/xm_local/packaging/bazel_tools.py b/xmanager/xm_local/packaging/bazel_tools.py
index 694f001..4dc52b0 100644
--- a/xmanager/xm_local/packaging/bazel_tools.py
+++ b/xmanager/xm_local/packaging/bazel_tools.py
@@ -147,7 +147,7 @@ def _build_multiple_targets(
 
 
 # Expansions (`...`, `*`) are not allowed.
-_NAME_RE = '[^:/.*]+'
+_NAME_RE = '[^:/*]+'
 _LABEL_LEXER = re.compile(
     f'^//(?P<packages>{_NAME_RE}(/{_NAME_RE})*)?(?P<target>:{_NAME_RE})?$')
 _LexedLabel = Tuple[List[str], str]
@@ -156,8 +156,10 @@ _LexedLabel = Tuple[List[str], str]
 def _lex_label(label: str) -> _LexedLabel:
   """Splits the label into packages and target."""
   match = _LABEL_LEXER.match(label)
-  if match is None:
-    raise ValueError(f'{label} is not an absolute Bazel label')
+  for g in match.groups('packages'):
+    print('group:', g)
+    if '..' in g:
+      raise ValueError(f'{label} is not an absolute Bazel label')
   groups = match.groupdict()
   packages: Optional[str] = groups['packages']
   target: Optional[str] = groups['target']
@dubov94 dubov94 self-assigned this Sep 29, 2022
@dubov94
Copy link
Member

dubov94 commented Sep 29, 2022

Hi Ryo,

Thanks for flagging!

I'm not a member of the team that builds XManager anymore, but as the author of that problem I'm willing to fix it nevertheless 🙂

In the meantime it can be avoided with Bazel's alias.

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

No branches or pull requests

2 participants