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

Handle dependency cycles #313

Merged
merged 5 commits into from
Jul 4, 2018
Merged

Conversation

mbargull
Copy link
Member

@mbargull mbargull commented Jul 4, 2018

The first three commits are some minor fixes.
The last two fix handling of dependencies by considering all requirements from build, host, as well as run environments. To avoid the previous workaround of only considering host due to cyclic dependencies of some existing recipes, this detects those dependency cycles and marks all packages of a cycle as failed (+ skipping any downstream packages, of course).

@@ -846,6 +845,16 @@ def check_recipe_skippable(recipe, channel_packages, force=False):
),
Counter()
)
if num_existing_pkg_builds == Counter():
# No packages with same version + build num in channels: no need to skip
return False
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If

num_existing_pkg_builds == num_new_pkg_builds == Counter()

this previously caused an additional

... the same number of builds are in channel(s) ...

info message to be displayed. But since

num_new_pkg_builds == Counter()

means there is nothing to do anyway,

Nothing to be done for recipe ...

is output in any case so that the former message is just noise.
ref: bioconda/bioconda-recipes#9686 (comment)

@@ -492,7 +492,7 @@ def get_deps(meta, sec):
deps = reqs.get(sec)
if not deps:
return []
return [dep.split()[0] for dep in deps if dep is not None]
return [dep.split()[0] for dep in deps if dep]
Copy link
Member Author

@mbargull mbargull Jul 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_meta_fast does not evaluate the Jinja functions. Hence, {{ compiler(...) }} or {{ pin_compatible(...) }} may be empty strings here.
Changing

if dep is not None

to

if dep

just prevents raising IndexError in dep.split()[0].
If we want to handle this correctly, we'd have to evaluate the Jinja constructs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine with me.

for cc_nodes in nx.connected_components(dag.to_undirected()):
cc = dag.subgraph(sorted(cc_nodes))
nodes_in_cycles = set()
for cycle in list(nx.simple_cycles(cc)):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some quick tests with the existing packages by clearing the build blacklist to get the cyclic dependency of the Perl packages that caused the troubles before.
Didn't write tests yet, though, sorry 🤦‍♂️ 🤕

@mbargull mbargull changed the title Handle dep cycles Handle dependency cycles Jul 4, 2018
@mbargull mbargull requested a review from johanneskoester July 4, 2018 16:28
@mbargull
Copy link
Member Author

mbargull commented Jul 4, 2018

NB: we should rename all dags to dependency_graph up until the point when we removed the cycles. Doesn't make much sense to call them DAGs 😆

@johanneskoester
Copy link
Contributor

@mbargull, I guess you will be busy the next days with travel preparation and the job application. I think it should be fine to merge this if you agree, although there is no test case yet.

@mbargull
Copy link
Member Author

mbargull commented Jul 4, 2018

Indeed. I believe it should work correctly and thus be fine to merge. Tests and cosmetics should be done later at some point. There are some more bits and pieces I noticed we will/might have to improve regarding the graph generation, nothing pressing though.

@mbargull
Copy link
Member Author

mbargull commented Jul 4, 2018

... and I completely forgot to link to issues these changes are fixing:
bioconda/bioconda-recipes#9457
bioconda/bioconda-recipes#9669

@johanneskoester johanneskoester merged commit b0df159 into cb3-migration Jul 4, 2018
@johanneskoester johanneskoester deleted the handle-dep-cycles branch July 4, 2018 21:34
@bgruening
Copy link
Member

Sorry but I think this broke the builds, e.g. https://circleci.com/gh/bioconda/bioconda-recipes/16499

  File "/home/circleci/project/miniconda/lib/python3.6/site-packages/conda/core/package_cache_data.py", line 605, in execute
    raise CondaMultiError(exceptions)
conda.CondaMultiError: Exceeded 30 redirects.

@mbargull
Copy link
Member Author

mbargull commented Jul 5, 2018

Very unlikely. That error seems unrelated. Possibly another issue with anaconda.org (those are pretty annoying lately..)?

@mbargull
Copy link
Member Author

mbargull commented Jul 5, 2018

Yep, looks like that has been some temporary fluke: The current bulk run is past the point were the previous one failed.

johanneskoester added a commit that referenced this pull request Jul 19, 2018
* Pin to conda-build 3.

* Depend on latest conda-forge-pinning package.

This will ensure that conda-build 3 is always using the pinnings defined in
https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/master/recipe/conda_build_config.yaml
By contributing to this list, we can maintain compatibility with conda-forge.

* Try downgrading to last minor release of conda.

* Try using conda 4.3.

* Try 4.5.0

* Handle UnableToParse

* Remove galaxy-lib version restriction.

* adapt to conda 4.5

* use get_output_file_paths instead of get_output_file_path

* Add lint for deprecated numpy spec.

* Document new lint.

* register lint.

* fix tests

* fix lint

* adapt test cases and remove obsolete ones.

* Remove env_matrix.

* fix typo

* fixes and further cleanup

* fixes

* Fixes.

* fix typo

* fixes

* fixes

* fixes.

* fix DAG

* Linting now uses fully rendered recipes, and checks all combinations of the variant matrix and both linux and osx.

* fix conda execution in docker

* fix argument

* fixes.

* Adapt numpy x.x lint to new multi-metadata strategy.

* properly render metadata when finding built package paths.

* fixes.

* fixes

* fix test for rendering sandboxing

* disable skip-existing

* Use same build args for the copying as for building.

* add test for conda-forge pinning

* add ability to have separate bioconda pins

* pep8

* add some more docs on testing

* add bioconda conda_build_config.yaml

* add first metadata fix

* update tests for load_first_metadata

* first pass of cb3 migration docs

* port bioconductor_skeleton to use cb3 compilers; improve detection

* update bioconductor_skeleton test

* more cb3 docs

* add anaconda to channels

* add anaconda channel to circleci setup

* short-circuit filtering if skipped

* fix text to only check names

* add test for when CI=true

* add extra checks that built pkgs exist

* add test for cb3 outputs

* clean up cb3 output test; typos

* utils.Progress: remove unnecessary sleep

* pkg_test: use --involucro-path to prevent download in galaxy-lib

* remove anaconda channel

* simplify htslib in test

* add compiler test

* fix htslib max pin

* update docs to reflect run_exports

* Update cb3.rst

* update docs

* add lint functions for compilers and fn

* add linting docs

* bring back the noarch lint docs

* render with finalize=False where possible

* rename load_conda_config to load_conda_build_config

* use conda.exports (public API)

* pin conda-forge-pinning, update conda/conda-build pins

* lint: handle if source is a list

* adjust tests

* show test durations

* lint: use decorator instead explicit loops

* lint: fix decorator, pass through __name__ for registry

* lint: fix decorator, pass through __name__ for registry (2)

* run long-running tests separately

* docker_utils: use base image acc. to BIOCONDA_UTILS_TAG

* Remove a priori filtering. Instead filter when each recipe is considered.

* Naming.

* Skip filter tests for now.

* Disable DAG generation.

* Activate DAG calculation again (we cannot avoid it because we need the topological sorting).

* Fast loading of metadata for DAG.

* Only use host dependencies when building DAG.

* get_package_paths returns tuple

* utils.get_package_paths: only return paths

* requirements: update conda, conda-build, conda-forge-pinning

* use bypass_env_check=True when we use non-finalized renderings

* requirements: use conda-build=3.10.3 for now

* test_utils: disable the remaining filter_recipes test

* Abort building if build strings in repo are divergent

* fix previous commit: add Python's redundant colon

* fix generate_docs.py

* fix another typo

* support linting just before building

* sort recipes for linting

* better noarch detection

* default df

* be clear which recipe the problem is on

* add back in debug info on linting

* more generic check for compilers

* add conda_build_config files consistently

* fix previous commit

* fix previous commit again

* fix mulled upload: iterate over all generated images.

* build: fix collecting mulled_images

* requirements: require python >=3.6

* build: skip builds early based on num of existing builds in channel

* build: topo-sort connected components before merging

* docs: handle extra.notes being a list

* build: handle UnsatisfiableError

* fix import

* [WIP] update to conda-build 3.10.9 (#312)

* build: rename --prelint to --lint, add --lint-only/--lint-exclude args

* requirements: update to conda-build 3.10.9

* build: remove default value for --lint-exclude

* requirements: require python >=3.6

* meta.get_section might return None (conda-build 3.10.9 compat)

* silence most flake8 nagging

* bioconductor: fix missing newline at EOF for post-link.sh

* fix typo

* use meta.get_value preferably

* make flake8 shut up already

* meta.get_value only works for path with depth 2

* lint: fix should_not_be_noarch for conda_build >3.10.3

* lint._has_compilers: also check for old compiler packages

* lint._get_deps: fix sections being unset on default

* docs: use only one job for debugging

* Add paper to the homepage.

* Handle dependency cycles (#313)

* fix minor bug in `build --lint-exclude`

* utils.check_recipe_skippable: avoid additional FILTER msg if nothing to do

* utils.get_dag: 'dep' can be empty string for unevaluated jinja expressions

* utils.get_dag: respect dependencies from build and run environments

* build.build_recipes: gracefully handle(=skip) packages in dependency cycles

* requirements: update to conda-forge-pinning 2018.07.18
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

Successfully merging this pull request may close these issues.

3 participants