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

Can some of the runtime dependencies be loosened? #44

Closed
bollwyvl opened this issue Oct 30, 2021 · 23 comments
Closed

Can some of the runtime dependencies be loosened? #44

bollwyvl opened this issue Oct 30, 2021 · 23 comments
Assignees
Labels
dependencies enhancement New feature or request

Comments

@bollwyvl
Copy link

Thanks for this!

I know poetry makes it easy lock every little thing down, and it makes testing easier, higher assurance, yadda yadda, but practically, it's quite inflexible when the effective ranges are very small... and on a self-declared lib to boot.

Specifically, hooray for declaring a setuptools dependency: so many pkgutils-using packages forget to.

However the size of the range covered by setuptools ^50.3.2 makes it relatively hard to appease (as in: exactly 1 version).

Selfishly, this is blocking me downstream in packaging this and ultimately jake 1.x for conda-forge.

The same goes for importlib_metadata which unfortunately gets pinned in a number of packages, and seems to change a lot for a backport package.

Anyhow: would the maintainers be open to a PR that:

  • loosened the range of e.g. setuptools to be something more like >=50.3.2,<59
  • added a CI test excursion for the lowest and highest bound

In the meantime, I may try patching the pin over on conda-forge and running the full test suite...

@bollwyvl
Copy link
Author

Welp, it looks like test suite passes locally with setuptools 58.3.2 with 93% coverage... i haven't dug into whether the missing lines are setuptools-relevant, but seems like a good sign:

https://github.com/conda-forge/staged-recipes/pull/16672/files#diff-3ac673913839cd48ed24c8f39f820bfaa5df9d6882b981e978d022ef548d0869R6

Some more logs will appear here:

https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=400079&view=logs&j=6f142865-96c3-535c-b7ea-873d86b887bd

@jkowalleck
Copy link
Member

jkowalleck commented Nov 7, 2021

did a quick lookup which parts of setuptools are actually used:

  • pkg_resources.working_set, pkg_resources.DistInfoDistribution
  • pkg_resources.parse_requirements(), pkg_resources.Requirement

did i miss something?


should be possible to go down to v47.0.0 - this is the version where py2 support was removed.
what do you think, @madpah ?


tested with a modified pyproject.toml and ran this in the venv:

    pip install poetry
    poetry install -v
    pip install -U setuptools==47.0.0
    python3 -m unittest discover -s tests -vv

test result:

Click to expand test results

test_bom_metadata_tool_multiple_tools (test_bom.TestBom) ... ok
test_bom_metadata_tool_this_tool (test_bom.TestBom) ... ok
test_bom_simple (test_bom.TestBom) ... ok
test_as_package_url_1 (test_component.TestComponent) ... ok
test_as_package_url_2 (test_component.TestComponent) ... ok
test_as_package_url_3 (test_component.TestComponent) ... ok
test_custom_package_url_type (test_component.TestComponent) ... ok
test_from_file_with_path_for_bom (test_component.TestComponent) ... ok
test_full_purl_spec_no_subpath (test_component.TestComponent) ... ok
test_get_component_by_purl_1 (test_component.TestComponent) ... ok
test_has_component_1 (test_component.TestComponent) ... ok
test_purl_correct (test_component.TestComponent) ... ok
test_purl_incorrect_name (test_component.TestComponent) ... ok
test_purl_incorrect_version (test_component.TestComponent) ... ok
test_purl_with_qualifiers (test_component.TestComponent) ... ok
test_json_defaults (test_e2e_environment.TestE2EEnvironment) ... ok
test_xml_defaults (test_e2e_environment.TestE2EEnvironment) ... ok
test_hash_type_from_composite_str_1 (test_model.TestModel) ... ok
test_hash_type_from_composite_str_2 (test_model.TestModel) ... ok
test_empty_basic_component (test_model_component.TestModelComponent) ... ok
test_external_references (test_model_component.TestModelComponent) ... ok
test_multiple_basic_components (test_model_component.TestModelComponent) ... ok
test_v_rating_scores_all (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_rating_scores_base_only (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_rating_scores_empty (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_severity_from_cvss_scores_multiple_critical (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_severity_from_cvss_scores_multiple_high (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_severity_from_cvss_scores_single_critical (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_severity_from_cvss_scores_single_high (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_severity_from_cvss_scores_single_low (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_severity_from_cvss_scores_single_medium (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_severity_from_cvss_scores_single_none (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_cvss2_1 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_cvss2_2 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_cvss2_3 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_cvss3_1 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_cvss3_2 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_cvss3_3 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_other_1 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_other_2 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_owasp_1 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_owasp_2 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_get_localised_vector_owasp_3 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_parse_cvss2_1 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_parse_cvss3_1 (test_model_vulnerability.TestModelVulnerability) ... ok
test_v_source_parse_owasp_1 (test_model_vulnerability.TestModelVulnerability) ... ok
test_get_instance_default (test_output_generic.TestOutputGeneric) ... ok
test_get_instance_xml (test_output_generic.TestOutputGeneric) ... ok
test_get_instance_xml_v1_3 (test_output_generic.TestOutputGeneric) ... ok
test_bom_v1_3_with_component_external_references (test_output_json.TestOutputJson) ... ok
test_bom_v1_3_with_component_hashes (test_output_json.TestOutputJson) ... ok
test_bom_v1_3_with_component_license (test_output_json.TestOutputJson) ... ok
test_simple_bom_v1_2 (test_output_json.TestOutputJson) ... ok
test_simple_bom_v1_3 (test_output_json.TestOutputJson) ... ok
test_bom_v1_3_with_component_external_references (test_output_xml.TestOutputXml) ... ok
test_bom_v1_3_with_component_hashes (test_output_xml.TestOutputXml) ... ok
test_simple_bom_v1_0 (test_output_xml.TestOutputXml) ... ok
test_simple_bom_v1_0_with_vulnerabilities (test_output_xml.TestOutputXml) ... ok
test_simple_bom_v1_1 (test_output_xml.TestOutputXml) ... ok
test_simple_bom_v1_2 (test_output_xml.TestOutputXml) ... ok
test_simple_bom_v1_3 (test_output_xml.TestOutputXml) ... ok
test_simple_bom_v1_3_with_vulnerabilities (test_output_xml.TestOutputXml) ... ok
test_with_component_license (test_output_xml.TestOutputXml) ... ok
test_conda_list_explicit_md5 (test_parser_conda.TestCondaParser) ... ok
test_conda_list_json (test_parser_conda.TestCondaParser) ... ok
test_simple (test_parser_environment.TestEnvironmentParser)
@todo This test is a vague as it will detect the unique environment where tests are being executed - ... ok
test_simple (test_parser_pipenv.TestPipEnvParser) ... ok
test_with_multiple_and_no_index (test_parser_pipenv.TestPipEnvParser) ... ok
test_simple (test_parser_poetry.TestPoetryParser) ... ok
test_example_1 (test_parser_requirements.TestRequirementsParser) ... ok
test_example_multiline_with_comments (test_parser_requirements.TestRequirementsParser) ... ok
test_example_with_comments (test_parser_requirements.TestRequirementsParser) ... ok
test_example_with_hashes (test_parser_requirements.TestRequirementsParser) ... skipped 'Not yet supported'
test_example_without_pinned_versions (test_parser_requirements.TestRequirementsParser) ... ok
test_simple (test_parser_requirements.TestRequirementsParser) ... ok
test_parse_conda_json_no_hash (test_utils_conda.TestUtilsConda) ... ok
test_parse_conda_list_str_no_hash (test_utils_conda.TestUtilsConda) ... ok
test_parse_conda_list_str_with_hash_1 (test_utils_conda.TestUtilsConda) ... ok
test_parse_conda_list_str_with_hash_2 (test_utils_conda.TestUtilsConda) ... ok
test_parse_conda_list_str_with_hash_3 (test_utils_conda.TestUtilsConda) ... ok

----------------------------------------------------------------------
Ran 80 tests in 0.250s

OK (skipped=1)

@jkowalleck
Copy link
Member

jkowalleck commented Nov 7, 2021

@madpah in preparation for this change I would recommend:

  • add type checkers to the Ci/CT - see typing & PEP 561 #59
  • modify CI/CT (github-workflow and tox) to run once with highest and once with lowest dependencies

@madpah
Copy link
Collaborator

madpah commented Nov 9, 2021

Happy to proceed with this - and thanks @bollwyvl for calling this out.

We'll get #59 merged first and then get the deps loosened up.

@jkowalleck
Copy link
Member

jkowalleck commented Nov 9, 2021

might be related: python-poetry/poetry#3527

@madpah
Copy link
Collaborator

madpah commented Nov 19, 2021

Just waiting on #71 / #68 before we loosen deps.

@jkowalleck
Copy link
Member

i reejected #71 / #68
@madpah feel free to go ahead.

@madpah
Copy link
Collaborator

madpah commented Nov 23, 2021

Thanks @jkowalleck.

Do we have a collective view as to how to handle CI for when we loosen dependency versions @jkowalleck? Doesn't seem like there is anything OOTB in Poetry to cover us here.

@madpah
Copy link
Collaborator

madpah commented Nov 23, 2021

Packages to consider:

  • setuptools
  • importlib_metadata
  • requirements_parser
  • packageurl-python
  • toml
  • typing-extensions
  • types-setuptools
  • types-toml

@madpah
Copy link
Collaborator

madpah commented Nov 23, 2021

@jkowalleck already researched setuptools - proposing we allow down to 47.0.0 and up to latest (59.2.0)

@madpah
Copy link
Collaborator

madpah commented Nov 23, 2021

importlib_metadata

requirements_parser
Appears not to be used and can be removed?

packageurl-python

  • We use PackageURL class
  • Looks like this class has been present with the same signature since the packages first release 0.3.0 (package-url/packageurl-python@bd63e2d) based on the date of this commit

toml

  • We use loads from toml
  • Tough to determine a safe lowest version - suggest we start with ^0.10.0

typing-extensions
Can stay as defined?

types-setuptools

  • First release was 57.0.0 - suggest we allow all versions?

types-toml

  • Use same version range as for toml?

@jkowalleck - thoughts on above?

@jkowalleck
Copy link
Member

jkowalleck commented Nov 23, 2021

great research, @madpah
now we could lower the requirements to the versions actually needed. :-)

at some point we should have CI to test once in latest/highest compatible setup, and once in lowest compatible setup.

issue i see: there is no way to setup lowest compatible environment
i mean, we run tests in a virtual environment (via tox) anyways, so we can downgrade some installs (setuptools, etc) and install dependencies in specific versions. that should be no issue.
but how could this be done, unless python-poetry/poetry#3527 was implemented ?

nevertheless, we might lower the dependencies, still.
tests for "lowest" can be added later. there are no tests for it now, and there is no proper way to do it at the moment, or is there?
@madpah what are your thoughts regarding tests? iwe might have a hand-crafted option - see #44 (comment)

@jkowalleck
Copy link
Member

jkowalleck commented Nov 23, 2021

Thanks @jkowalleck.

Do we have a collective view as to how to handle CI for when we loosen dependency versions @jkowalleck? Doesn't seem like there is anything OOTB in Poetry to cover us here.

could not find anything.
since we discuss direct dependencies at this point of view, we could do the following:

from pyproject.toml i found our direct dependencies:

[tool.poetry.dependencies]
python = "^3.6"
packageurl-python = "^0.9.4"
requirements_parser = "^0.2.0"
setuptools = "^50.3.2"
importlib-metadata = { version = "^4.8.1", python = "~3.6 | ~3.7" }
toml = "^0.10.2"
typing-extensions = { version = "^3.10.0", python = "~3.6 | ~3.7" }
types-setuptools = "^57.4.2"
types-toml = "^0.10.1"

so we could have a requirements.lowest.txt with the following content

# exactly pinned dependencies to lowest 
# see pyptoject file for ranges
packageurl-python == 0.9.4
requirements_parser == 0.2.0
setuptools == 50.3.2
importlib-metadata == 4.8.1; python_version < "3.8" 
toml == 0.10.2
typing-extensions == 3.10.0 ; python_version < "3.8" 
types-setuptools == 57.4.2
types-toml == 0.10.1

which in turn could be installed in a CI/tox setup right after poetry install installed its needed tools.
something like pip install -r requirements.lowest.txt will do
this way we would have the lowest versions available

@jkowalleck
Copy link
Member

jkowalleck commented Nov 23, 2021

prepared a setup as descibed: #79

@madpah lets see if we like the setup i built, and if we can fix it.
then we would have a test bed in which we could lower the dependencies as planned.

@madpah
Copy link
Collaborator

madpah commented Dec 9, 2021

Note: importlib-metadata is not a typed library until version 3.4.0, regardless of functionallity.

Will use this as the minimum.

@madpah
Copy link
Collaborator

madpah commented Dec 9, 2021

Unfortunately version 3.4.0 of importlib-metadata doesn't make importlib_metadata._meta.PackageMetadata public - will try 4.1.0 as the lowest to resolve this.

See commit on importlib_metadata.

@jkowalleck
Copy link
Member

jkowalleck commented Dec 9, 2021

Unfortunately version 3.4.0 of importlib-metadata doesn't make importlib_metadata._meta.PackageMetadata public - will try 4.1.0 as the lowest to resolve this.

See commit on importlib_metadata.

importlib_metadata._meta.PackageMetadata ... importlib_metadata._meta is not intended to be public anyways.
according to https://github.com/python/importlib_metadata/blob/v3.4.0/importlib_metadata/__init__.py
there is importlib_metadata.PackageMetadata ...

made available with the same name importlib_metadata._meta.PackageMetadata in https://github.com/python/importlib_metadata/blob/v4.8.2/importlib_metadata/__init__.py

i introduced the wrong usage of importlib_metadata._meta.PackageMetadata when we fixed some typings.
i actually looked up the correct type inport (as mentioned above) but forgot to change the import.

sorry for that, @madpah

@jkowalleck jkowalleck linked a pull request Dec 9, 2021 that will close this issue
@madpah
Copy link
Collaborator

madpah commented Dec 9, 2021

Indeed - will update and lower back to 3.4.0.

@madpah madpah self-assigned this Dec 9, 2021
@madpah
Copy link
Collaborator

madpah commented Dec 9, 2021

#89 has been merged.

@madpah
Copy link
Collaborator

madpah commented Dec 9, 2021

@bollwyvl - sorry this has taken a little while to organise, but we've just released 0.12.0 which has loosened the dependency versions significantly.

Would appreciate your feedback!

jkowalleck added a commit that referenced this issue Dec 9, 2021
see #44

Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
jkowalleck added a commit that referenced this issue Dec 9, 2021
see #44

Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
jkowalleck added a commit that referenced this issue Dec 9, 2021
see #44

Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
jkowalleck added a commit that referenced this issue Dec 9, 2021
see #44

updated some locked dependencies to latest versions

Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
@bollwyvl
Copy link
Author

bollwyvl commented Dec 9, 2021 via email

jkowalleck added a commit that referenced this issue Dec 9, 2021
see #44

updated some locked dependencies to latest versions

Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
madpah pushed a commit that referenced this issue Dec 9, 2021
see #44

updated some locked dependencies to latest versions

Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
@bollwyvl
Copy link
Author

bollwyvl commented Dec 9, 2021

Yep, was able to ship 0.12.0 without any patches, and all the tests passed the first time, just the way we likes it! Thanks again.

@madpah
Copy link
Collaborator

madpah commented Dec 15, 2021

That's epic @bollwyvl. Thanks for confirming.

We've of course pushed a few tweaks since 0.12.0 as we've worked through a few niggles - but good to know this is now solved for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants