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 a config/command line option to treat some downstream errors as warning instead of complete error exit #478

Open
norg opened this issue Jan 12, 2023 · 19 comments
Assignees
Labels
component:dep-sources Dependency sources enhancement New feature or request

Comments

@norg
Copy link

norg commented Jan 12, 2023

Is your feature request related to a problem? Please describe.

From time to time we run into issues where pip-audit stops working with a traceback or error that is not related to pip-audit itself but for reasons in the projects that are audited.

So for example celery is not following PEP440 currently for the requirements.txt, this results in a traceback when running pip-audit since packaging is now more strict on that. So until celery is changing the file a pip-audit run that has a dependency that at one point ends up with an issue, the whole audit breaks.

I would argue that his can happen more often and a user would still like to have a report for the packages that are resolved.

Describe the solution you'd like

Ideally the user can configure this and pip-audit will proceed with the rest of the audit and report a warning that some parts couldn't be audited due to errors.

The advantage of the config option is, that the default could still be a clear error and exit by pip-audit but users who would like to have a partial report have an alternative, especially if the proper fix in the other package might take time or isn't even considered.

Describe alternatives you've considered

Reporting the issues to the project is the proper way but until some things are fixed pip-audit runs would always fail.

Additional context

Example in Celery celery/celery#8001 (comment) which is detected by packaging pypa/packaging#658

@norg norg added the enhancement New feature or request label Jan 12, 2023
@woodruffw
Copy link
Member

woodruffw commented Jan 12, 2023

Thanks for the report!

To clarify, the problem is requirements specifiers like this, right?

pytz (>dev)

Could you also post the pip-audit traceback you're seeing for that?

Given that we've jumped a bit ahead of pip and the rest of the Python ecosystem in terms of using the latest packaging version, I agree that it probably makes sense for us to add an option/configuration setting that allows us to warn and skip rather than fail on non-PEP 440 specifiers.

(Alternatively, we could be a forward-pushing force, and carve out a specific error here telling users that they're going to need to fix their dependencies soon anyways.)

cc @di for thoughts.

@norg
Copy link
Author

norg commented Jan 12, 2023

To clarify, the problem is requirements specifiers like this, right?

pytz (>dev)

Yep, exactly this was one of the issues. But I remember that for example an older version of python-ldap didn't work with the pip in a CI environment and thus the whole run was also killed. So I guess there are several scenarios where an error deeper in the chain result in an overall error/exit that could be treated as a warning and "skipped" with a note.

Could you also post the pip-audit traceback you're seeing for that?

Traceback (most recent call last):
  File "/home/andi/.local/lib/python3.10/site-packages/packaging/requirements.py", line 35, in __init__
    parsed = parse_requirement(requirement_string)
  File "/home/andi/.local/lib/python3.10/site-packages/packaging/_parser.py", line 64, in parse_requirement
    return _parse_requirement(Tokenizer(source, rules=DEFAULT_RULES))
  File "/home/andi/.local/lib/python3.10/site-packages/packaging/_parser.py", line 82, in _parse_requirement
    url, specifier, marker = _parse_requirement_details(tokenizer)
  File "/home/andi/.local/lib/python3.10/site-packages/packaging/_parser.py", line 120, in _parse_requirement_details
    specifier = _parse_specifier(tokenizer)
  File "/home/andi/.local/lib/python3.10/site-packages/packaging/_parser.py", line 206, in _parse_specifier
    with tokenizer.enclosing_tokens("LEFT_PARENTHESIS", "RIGHT_PARENTHESIS"):
  File "/usr/lib/python3.10/contextlib.py", line 142, in __exit__
    next(self.gen)
  File "/home/andi/.local/lib/python3.10/site-packages/packaging/_tokenizer.py", line 183, in enclosing_tokens
    self.raise_syntax_error(
  File "/home/andi/.local/lib/python3.10/site-packages/packaging/_tokenizer.py", line 163, in raise_syntax_error
    raise ParserSyntaxError(
packaging._tokenizer.ParserSyntaxError: Expected closing RIGHT_PARENTHESIS
    pytz (>dev)
         ~^

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/andi/.local/bin/pip-audit", line 8, in <module>
    sys.exit(audit())
  File "/home/andi/.local/lib/python3.10/site-packages/pip_audit/_cli.py", line 455, in audit
    for (spec, vulns) in auditor.audit(source):
  File "/home/andi/.local/lib/python3.10/site-packages/pip_audit/_audit.py", line 67, in audit
    for dep, vulns in self._service.query_all(specs):
  File "/home/andi/.local/lib/python3.10/site-packages/pip_audit/_service/interface.py", line 155, in query_all
    for spec in specs:
  File "/home/andi/.local/lib/python3.10/site-packages/pip_audit/_dependency_source/requirement.py", line 116, in collect
    for _, dep in self._collect_cached_deps(filename, reqs):
  File "/home/andi/.local/lib/python3.10/site-packages/pip_audit/_dependency_source/requirement.py", line 328, in _collect_cached_deps
    for req, resolved_deps in self._resolver.resolve_all(iter(req_values)):
  File "/home/andi/.local/lib/python3.10/site-packages/pip_audit/_dependency_source/interface.py", line 88, in resolve_all
    yield (req, self.resolve(req))
  File "/home/andi/.local/lib/python3.10/site-packages/pip_audit/_dependency_source/resolvelib/resolvelib.py", line 77, in resolve
    result = self.resolver.resolve([req])
  File "/usr/lib/python3.10/site-packages/resolvelib/resolvers.py", line 521, in resolve
    state = resolution.resolve(requirements, max_rounds=max_rounds)
  File "/usr/lib/python3.10/site-packages/resolvelib/resolvers.py", line 402, in resolve
    failure_causes = self._attempt_to_pin_criterion(name)
  File "/usr/lib/python3.10/site-packages/resolvelib/resolvers.py", line 238, in _attempt_to_pin_criterion
    criteria = self._get_updated_criteria(candidate)
  File "/usr/lib/python3.10/site-packages/resolvelib/resolvers.py", line 228, in _get_updated_criteria
    for requirement in self._p.get_dependencies(candidate=candidate):
  File "/home/andi/.local/lib/python3.10/site-packages/pip_audit/_dependency_source/resolvelib/pypi_provider.py", line 415, in get_dependencies
    return candidate.dependencies
  File "/home/andi/.local/lib/python3.10/site-packages/pip_audit/_dependency_source/resolvelib/pypi_provider.py", line 126, in dependencies
    self._dependencies = list(self._get_dependencies())
  File "/home/andi/.local/lib/python3.10/site-packages/pip_audit/_dependency_source/resolvelib/pypi_provider.py", line 112, in _get_dependencies
    r = Requirement(d)
  File "/home/andi/.local/lib/python3.10/site-packages/packaging/requirements.py", line 37, in __init__
    raise InvalidRequirement(str(e)) from e
packaging.requirements.InvalidRequirement: Expected closing RIGHT_PARENTHESIS
    pytz (>dev)

Given that we've jumped a bit ahead of pip and the rest of the Python ecosystem in terms of using the latest packaging version, I agree that it probably makes sense for us to add an option/configuration setting that allows us to warn and skip rather than fail on non-PEP 440 specifiers.

At least for sometime, I guess most active projects will be forced to follow that.

(Alternatively, we could be a forward-pushing force, and carve out a specific error here telling users that they're going to need to fix their dependencies soon anyways.)

I agree, that it should be clearly pointed out, ideally with more details what to do. So a red warning sign to make sure it's getting fixed :) And by default error out might be fine as well, but sometimes you want to run audits while you're waiting for a fix upstream and are fine if one dependency is currently not fully covered but at least all the others still are.

@woodruffw
Copy link
Member

Thanks! And yeah, 100% agreed -- IMO pip-audit should always be "strict" by default (despite the confusing --strict flag), but in situations like this where there are significant ecosystem changes we should aim to match/allow by configuration what pip et al. do now, rather than what they will do in the future.

@woodruffw woodruffw added the component:dep-sources Dependency sources label Jan 12, 2023
@norg
Copy link
Author

norg commented Jan 30, 2023

Do you have any suggestion what one should do in the meantime to workaround this?

My only idea is to use an older pip-audit version in the pipeline for now which works with the older packaging.

I reported the issue upstream and they fixed it in master, but the release to pip might take some time.

@di
Copy link
Member

di commented Jan 30, 2023

Short term, downgrading would be the best workaround. We're unlikely to provide support for auditing projects that aren't standards-compliant, but we should definitely be providing a better exception here, or even better, skipping & warning on these invalid projects.

(Generally I don't think any packaging exceptions should become visible to pip-audit end users if we're doing things right).

@woodruffw
Copy link
Member

Yeah, we can definitely do much better with the error message here. I'll look into that now.

@woodruffw
Copy link
Member

Just enumerating; there are two separate improvements we can make here:

  1. When top-level requirements are invalid (e.g. pytz (>dev) in a top-level requirements.txt), we can improve the current DependencySourceError message's rendering (including rendering the exact parse location at which the requirements parser encountered an error).
  2. When we see an invalid transitive requirement, we should trickle up a better (and custom) error versus just the packaging one.

@woodruffw
Copy link
Member

@norg do you have a reproducible case you can share? I can cause the top-level error with:

pip-audit -r <(echo 'pytz (>dev)')

...but not the nested error you're seeing with celery.

@norg
Copy link
Author

norg commented Jan 30, 2023

@norg do you have a reproducible case you can share? I can cause the top-level error with:

pip-audit -r <(echo 'pytz (>dev)')

...but not the nested error you're seeing with celery.

Sure, you can trigger this when you use an older version and specify it:

pip-audit -r <(echo 'celery==4.4.7')

This does trigger the Traceback for me on pip-audit version 2.4.14 (which is caused by the same pytz error). I guess this is the nested case you want to match against.

Thanks for looking into this!

@woodruffw
Copy link
Member

This does trigger the Traceback for me on pip-audit version 2.4.14 (which is caused by the same pytz error). I guess this is the nested case you want to match against.

Indeed, thanks!

@woodruffw
Copy link
Member

Just to show the WIP (I'm going to clean it up a bit more):

$ pip-audit -r <(echo 'celery==4.4.7')
ERROR:pip_audit._cli:celery has invalid requirement pytz (>dev): Expected closing RIGHT_PARENTHESIS
    pytz (>dev)
         ~^

@woodruffw
Copy link
Member

#507 improves the error messages here, but we've left this open since it's not a "full" fix. Longer term, we should consider re-adding support for legacy (non-PEP 440) versions, explicitly skipping those versions, or making the current implicit policy of always using the latest packaging standards more explicit.

@woodruffw
Copy link
Member

why you guys always break everything

Please be substantive. This is a year old issue, we don't know what you think broke.

@Simanas
Copy link

Simanas commented Jan 18, 2024

Upgraded to the latest pip and now I am basically unable to do anything as I am getting exception on any attempt to install or upgrade one of my packages that is dependent on Django and Celery... So what do I do?

@pypa pypa deleted a comment from leeming87v5 Jan 18, 2024
@woodruffw
Copy link
Member

Upgraded to the latest pip and now I am basically unable to do anything as I am getting exception on any attempt to install or upgrade one of my packages that is dependent on Django and Celery... So what do I do?

Could you paste the full error you're seeing, and share the following:

  • The set of dependencies you're trying to install;
  • Your Python version;
  • Your pip and pip-audit versions

It's possible this is an upstream change in pip that we have limited control over: if pip is now strictly enforcing PEP 440 specifiers, then pip-audit has limited ability to override that change. But more context will help us determine if that's the case.

@woodruffw woodruffw self-assigned this Jan 18, 2024
@Simanas
Copy link

Simanas commented Jan 19, 2024

I am not sure who is responsible for this as I am not deeply familiar to the inner workings of pip, pip-audit, setuptools, etc.., however... whoever introduced this is stupid beyond imagination!

Now... what else do you want to be provided..? It's as simple as it can be!

It goes like this... somebody tries to install some pip package and he is faced with some pip errors, ok, so he starts googling... the very first advise that he gets on the internet is to upgrade pip and setuptools to the latest version. Once he does that... he is screwed, as he starts getting this error all over the place!

For example my project has dependency on an older celery packge, I think it was celery==4.4.7. But that's my own package that I maintain myself. Well ok, so I try and upgrade that requirement to celery==5.3.6. Well.. ok it kind of helps, however it also depends on librosa==0.10.1, which subsequently depends on more packages, that are older... which eventually brakes with same message...

So basically this is a disaster, as usually you are not in control of those dependencies, you just simply want to install some packages, which are sometimes a few years old and not necessarily meets latest PEP requirements, however they work perfectly well! And people sometimes have been using them for years without any trouble! Well but after you upgrade your pip... you start having serious troubles! So what's the point of upgrading it?

This is not how it should be executed!!! If there's PEP requirement enforcement planned ahead it must be performed on builders side, so that whenever you want to build your package in to a wheel and publish it on to PyPy things would brake and you would not be allowed to do so until you fix your own build, but not wise versa!

This is not full story however... We have a big project, with hundreds of installations that has auto updating system using pip to install dependencies... So when this happens... you are in a seriously deep shit!

To remediate this I had to enforce duck tape solution: pip install setuptools==65.7.0 before we start dealing with pip.

Sorry, for my emotions, but I am really pissed off by this. This is not how things should be done in Python world. I am proud Python developer for last 20 years, but this really got me seriously upset.

@woodruffw
Copy link
Member

I am not sure who is responsible for this as I am not deeply familiar to the inner workings of pip, pip-audit, setuptools, etc.., however... whoever introduced this is stupid beyond imagination!

We don't know what "this" is. You were asked to provide additional context in #478 (comment), and haven't done so. We cannot help you resolve your problem until we know what the actual problem is.

To set a clear expectation here: this is pip-audit, not pip or setuptools or anything else. This is not an issue tracker for those projects, and reports of problems with those projects cannot be filed productively here. If your problem is not a problem in pip-audit, then we have no direct ability to address it. Even if we could address it, this is not an appropriate or productive way to express your frustrations. If you want to continue to interact on this issue tracker, you'll need to calmly and productively work with us to figure out three things:

  1. What the actual issue is, by following the triage steps above (and any additional steps we may need for debugging);
  2. Whether the issue can be fixed on pip-audit's side;
  3. How we can fix it, if it can be fixed on our side.

@Simanas
Copy link

Simanas commented Jan 19, 2024

Need more context to move your a**? @woodruffw you are as ignorant as f***! So it is not approporiate, nor it is humane, nor colaborative, nor helping nor healthy, nor sustainable, nor friendly, nor productive.., etc... That's more than enough context for you to start making any progress, if it's even possible for you to comprehend.

@woodruffw
Copy link
Member

Okay, thanks for making this decision easy.

@pypa pypa locked as off-topic and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component:dep-sources Dependency sources enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants