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

[Housekeeping] Remove unused retry dependency in flytekit #3052

Closed
2 tasks done
hajapy opened this issue Nov 4, 2022 · 1 comment
Closed
2 tasks done

[Housekeeping] Remove unused retry dependency in flytekit #3052

hajapy opened this issue Nov 4, 2022 · 1 comment
Labels
flytekit FlyteKit Python related issue good first issue Good for newcomers housekeeping Issues that help maintain flyte and keep it tech-debt free

Comments

@hajapy
Copy link

hajapy commented Nov 4, 2022

Describe the issue

Remove the unused retry library dependency declared by flytekit: https://github.com/flyteorg/flytekit/blob/3b5d15acb5389d99b86ba342a77e1cab5ce8546d/setup.py#L68

This library is not used anywhere within flytekit that I can find searching for import retry or from retry, except in the sagemaker plugin: https://github.com/flyteorg/flytekit/blob/3b5d15acb5389d99b86ba342a77e1cab5ce8546d/plugins/flytekit-aws-sagemaker/flytekitplugins/awssagemaker/distributed_training.py#L8

The dependency could be moved to the sagemaker plugin's requirements so only users of that plugin need it installed. Though, given the unmaintained state of the retry library, I'd advise against that and suggest to look for a maintained alternative (reretry? https://github.com/leshchenko1979/reretry)

What if we do not do this?

All users of flytekit end up installing an unmaintained library (see invl/retry#36, not updated since 2016), which also ends up bringing with it unnecessary dependency on py which itself has been the source of lots of security alerting pytest-dev/py#287

Related component(s)

flytekit

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@hajapy hajapy added the housekeeping Issues that help maintain flyte and keep it tech-debt free label Nov 4, 2022
@pingsutw pingsutw added the flytekit FlyteKit Python related issue label Nov 9, 2022
@eapolinario eapolinario added the good first issue Good for newcomers label Nov 9, 2022
@mattb-zip
Copy link

there is a maintained fork of retry that addresses this CVE in version 0.9.5 and removes the py dependency, which is actually unused:

https://pypi.org/project/retry2/#history

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flytekit FlyteKit Python related issue good first issue Good for newcomers housekeeping Issues that help maintain flyte and keep it tech-debt free
Projects
None yet
Development

No branches or pull requests

4 participants