Poetry: [InvalidRequirement] Invalid requirement, parse error at "'extra =='"

[simonepri]

• [x] I am on the latest Poetry version.
• [x] I have searched the issues of this repo and believe that this is not a duplicate.
• [x] If an exception occurs when executing a command, I executed it again in debug mode (-vvv option).

• OS version and name: maxOS 10.15
• Poetry version: 1.0.5
  

Issue

When I run:

poetry add git+https://github.com/huggingface/transformers.git#a21d4fa410dc3b4c62f93aa0e6bbe4b75a101ee9 -vvv

The following exception is thrown:

[InvalidRequirement]
Invalid requirement, parse error at "'extra =='"

Traceback (most recent call last):
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/vendor/lib/python3.7/site-packages/clikit/console_application.py", line 131, in run
    status_code = command.handle(parsed_args, io)
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/vendor/lib/python3.7/site-packages/clikit/api/command/command.py", line 120, in handle
    status_code = self._do_handle(args, io)
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/vendor/lib/python3.7/site-packages/clikit/api/command/command.py", line 171, in _do_handle
    return getattr(handler, handler_method)(args, io, self)
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/vendor/lib/python3.7/site-packages/cleo/commands/command.py", line 92, in wrap_handle
    return self.handle()
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/lib/python3.7/site-packages/poetry/console/commands/add.py", line 89, in handle
    packages, allow_prereleases=self.option('allow-prereleases')
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/lib/python3.7/site-packages/poetry/console/commands/init.py", line 293, in _determine_requirements
    requires = self._parse_requirements(requires)
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/lib/python3.7/site-packages/poetry/console/commands/init.py", line 381, in _parse_requirements
    "git", url.url, reference=pair.get("rev")
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/lib/python3.7/site-packages/poetry/puzzle/provider.py", line 204, in get_package_from_vcs
    package = cls.get_package_from_directory(tmp_dir, name=name)
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/lib/python3.7/site-packages/poetry/puzzle/provider.py", line 412, in get_package_from_directory
    dep = dependency_from_pep_508(req)
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/lib/python3.7/site-packages/poetry/packages/__init__.py", line 39, in dependency_from_pep_508
    req = Requirement(name)
  File "/usr/local/Cellar/poetry/1.0.5_1/libexec/lib/python3.7/site-packages/poetry/version/requirements.py", line 212, in __init__
    requirement_string[e.loc : e.loc + 8]

If I try to add the package manually to the pyproject.toml file as follows:

transformers = {git = "https://github.com/huggingface/transformers.git", rev = "a21d4fa410dc3b4c62f93aa0e6bbe4b75a101ee9"}

The following exception is thrown instead:

[SolverProblemError]
Because text2error depends on transformers (*) which doesn't exist, version solving failed.

Ref https://github.com/huggingface/transformers/issues/3982

[abn]

@simonepri I was able to use the add command without errors on a linux machine. I suspect this might be an issue with transformer setup.py failing and poetry falling back to the ast parser. The ast parser is known to have issues with complex setup.py files.

Could you see if you can execute python setup.py egg_info within a clone of transformers? This should help isolate the problem, more so confirm the above.

[simonepri]

I was able to use the add command without errors on a linux machine.

I tried on Ubuntu and it produces the same error: https://colab.research.google.com/drive/1xUMhXL1iPiJjAgLMAN8OzJk21SGGhGQR

Could you see if you can execute python setup.py egg_info within a clone of transformers? This should help isolate the problem, more so confirm the above.

On macOS running:

git clone https://github.com/huggingface/transformers
cd transformers
git checkout a21d4fa410dc3b4c62f93aa0e6bbe4b75a101ee9
python setup.py egg_info

gives the following:

running egg_info
creating src/transformers.egg-info
writing src/transformers.egg-info/PKG-INFO
writing dependency_links to src/transformers.egg-info/dependency_links.txt
writing requirements to src/transformers.egg-info/requires.txt
writing top-level names to src/transformers.egg-info/top_level.txt
writing manifest file 'src/transformers.egg-info/SOURCES.txt'
reading manifest file 'src/transformers.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
writing manifest file 'src/transformers.egg-info/SOURCES.txt'

[simonepri]

git+git://github.com/timothycrosley/isort.git@e63ae06ec7d70b06df9e528357650281a3d3ec22; extra == "dev"

This is the value of requirement_string for which the REQUIREMENT pyparsing grammar fails.

https://github.com/python-poetry/poetry/blob/e8b6e15ecfe554b3c3d5f63983330fcddd7f59ca/poetry/version/requirements.py#L207-L214

[simonepri]

It seems that when a url is provided the REQUIREMENT grammar (mistakenly?) expects a space before the ;.

cc: @sdispater

[abn]

@simonepri most of that was copied over from the packaging project.

I believe it is a side-effect of using egg-info fallback here. When poetry parses the requires.txt generated; it converts the following

[dev]
...
isort@ git+git://github.com/timothycrosley/isort.git@e63ae06ec7d70b06df9e528357650281a3d3ec22#egg=isort
...

To this;

isort@ git+git://github.com/timothycrosley/isort.git@e63ae06ec7d70b06df9e528357650281a3d3ec22#egg=isort; extra == "dev"

As per PEP-508;

The "extra" variable is special. It is used by wheels to signal which specifications apply to a given extra in the wheel METADATA file, but since the METADATA file is based on a draft version of PEP-426, there is no current specification for this. Regardless, outside of a context where this special handling is taking place, the "extra" variable should result in an error like all other unknown variables.

The wheel METADATA will contain this;

Requires-Dist: isort @ git+git://github.com/timothycrosley/isort.git@e63ae06ec7d70b06df9e528357650281a3d3ec22#egg=isort ; extra == 'dev'

The fix here, I think should be to ensure we add a whitespace prefix for the semicolon as expected.

https://github.com/python-poetry/core/blob/8f360360913e0a935bdcf04e0c36b5faee33491b/poetry/core/utils/helpers.py#L83-L85

[simonepri]

The fix here, I think should be to ensure we add a whitespace prefix for the semicolon as expected.

After the fix you suggested, the parse_requires function returns:

isort@ git+git://github.com/timothycrosley/isort.git@e63ae06ec7d70b06df9e528357650281a3d3ec22#egg=isort ; extra == "dev"

But then this string is fed into dependency_from_pep_508 which changes it to

isort@ git+git://github.com/timothycrosley/isort.git@e63ae06ec7d70b06df9e528357650281a3d3ec22; extra == "dev"

Thus we also need to change this:

https://github.com/python-poetry/core/blob/6d0f8fdd8cde3959cd347cbd0060fdede6e097d1/poetry/core/packages/__init__.py#L66-L67

[simonepri]

@abn I noticed that all the tests strings do not have a space before the ;.

https://github.com/python-poetry/core/blob/3ccd963c724121939a3f11aa73dc543ea1293fad/tests/packages/test_main.py

Shall I change them as well together with the new PR?

The reason why they all pass is that the REQUIREMENT grammar does not need the space before the ; if what comes before is not an URL.

It seems weird to me that we have a grammar that allows for both
requests==2.18.0; extra == "foo" and requests==2.18.0 ; extra == "foo", but it does not allow for django-utils @ https://example.com/django-utils-1.0.0.tar.gz; extra == "foo".

[simonepri]

Also, I couldn't find any tests for the parse_requires function.
Are there any?

[abn]

I agree that the grammar could be improved, but at the moment I am reluctant to make changes to that module since I am not entirely sure if we should replicate the logic and would like to look at reusing package.requirements instead, but this is something that need to discussed elsewhere.

For now, I am inclined to keep the grammar as is until we decide on how we should proceed on the broader case.

As for tests, I would retain the existing tests, but also add extra ones to ensure that a whitespace is also supported.

The parse_requires function might not already have coverage; I'd recommend creating a requires.txt fixture with a few different cases. I know this might be more than what we need here, but it is a good opportunity to expand the test suite. I'd add this only for the core PR as the tests.packages.test_main suite should be sufficient for the bug fix in master.

[simonepri]

but at the moment I am reluctant to make changes to that module since I am not entirely sure if we should replicate the logic and would like to look at reusing package.requirements instead

I agree that reusing their code makes a lot of sense.

Anyway, I think is worth opening an issue on their side to ask wether that is intentional or not.

If you look at their test they indeed add a space for urls:
https://github.com/pypa/packaging/blob/61672bf9f507f38e84ce2786a1c42f55fa0a3153/tests/test_requirements.py#L26

But then in the __str__ method they do not add the space back after the marker.
https://github.com/pypa/packaging/blob/61672bf9f507f38e84ce2786a1c42f55fa0a3153/packaging/requirements.py#L139

[abn]

But then in the __str__ method they do not add the space back after the marker.
https://github.com/pypa/packaging/blob/61672bf9f507f38e84ce2786a1c42f55fa0a3153/packaging/requirements.py#L139

Our fix is simply working around the issue. The root issue is definitely the grammar, the regex is a bit too greedy and should be fixed eventually. Let's see what happens with the issue upstream.

[simonepri]

The root issue is definitely the grammar

I think the reason why the grammar behaves like that is because of the following example, that should be a valid url:

poetry @ git+https://github.com/python-poetry/poetry.git@b;ar; ; extra == "foo;"

But then in the __str__ method they do not add the space back after the marker.
https://github.com/pypa/packaging/blob/61672bf9f507f38e84ce2786a1c42f55fa0a3153/packaging/requirements.py#L139

I was actually wrong, they do add the space when they stringify the object, the thing is that they only do that for URLs.

https://github.com/pypa/packaging/blob/61672bf9f507f38e84ce2786a1c42f55fa0a3153/packaging/requirements.py#L136

[simonepri]

The parse_requires function might not already have coverage; I'd recommend creating a requires.txt fixture with a few different cases. I know this might be more than what we need here, but it is a good opportunity to expand the test suite. I'd add this only for the core PR as the tests.packages.test_main suite should be sufficient for the bug fix in master.

It is tested in python-poetry. Should we add the tests to core as well?
https://github.com/python-poetry/poetry/blob/299a885e880afce613f76927165dce3b1f55591c/tests/utils/test_helpers.py

[abn]

Yes please.