Skip to content

Fixing a bug in which filtered dependencies based on complex filterin…#2983

Merged
bartoldeman merged 6 commits intoeasybuilders:developfrom
ComputeCanada:filter_deps_fix
Sep 9, 2019
Merged

Fixing a bug in which filtered dependencies based on complex filterin…#2983
bartoldeman merged 6 commits intoeasybuilders:developfrom
ComputeCanada:filter_deps_fix

Conversation

@mboisson
Copy link
Copy Markdown
Contributor

@mboisson mboisson commented Aug 26, 2019 •
edited by boegel
Loading

I moved much of the filter_deps code to "dep_is_filtered" function, and reuse that function in _finalize_dependencies, which was only checking for the name of the dependency, instead of the full version check.

@mboisson
Fixing a bug in which filtered dependencies based on complex filterin…
3c86935 
…g schema (specific versions) were not being filtered.
houndci-bot
houndci-bot reviewed Aug 26, 2019
View reviewed changes
Comment thread easybuild/framework/easyconfig/easyconfig.py Outdated
Comment thread easybuild/framework/easyconfig/easyconfig.py Outdated
Comment thread easybuild/framework/easyconfig/easyconfig.py Outdated
Comment thread easybuild/framework/easyconfig/easyconfig.py
@bartoldeman
Copy link
Copy Markdown
Contributor

bartoldeman commented Aug 27, 2019

I wonder if you can write a test for this bug as well?

@mboisson
Copy link
Copy Markdown
Contributor Author

mboisson commented Aug 27, 2019

I'm not quite sure how to trigger the bug actually. It showed up for some dependencies on CMake when we changed from filter-deps = ...,CMake,... to filter-deps = ...,CMake=:3.12.3[,...

I found the code path only by deduction. I know that this change gets rid of the bug that we were triggering.

@boegel boegel added the bug fix label Aug 27, 2019
@boegel boegel added this to the 4.0.0 milestone Aug 27, 2019
@boegel
Copy link
Copy Markdown
Member

boegel commented Aug 27, 2019

This definitely needs a test before it gets merged (or we will be bitten by this again sooner or later).

@boegel
Copy link
Copy Markdown
Member

boegel commented Aug 27, 2019

@mboisson How did the bug manifest itself exactly? Was it via an error like Failed to determine minimal toolchain for dep?

@easybuilders easybuilders deleted a comment from boegelbot Aug 29, 2019
@easybuilders easybuilders deleted a comment from boegelbot Aug 29, 2019
@bartoldeman
Copy link
Copy Markdown
Contributor

bartoldeman commented Sep 5, 2019

Reopening to see if mysterious py 3.5 failure is reproduced.

@bartoldeman bartoldeman closed this Sep 5, 2019
@bartoldeman bartoldeman reopened this Sep 5, 2019
@bartoldeman
Copy link
Copy Markdown
Contributor

bartoldeman commented Sep 6, 2019

@mboisson I merged your branch with develop to see because it was a little out of sync with the Py3 changes. On build-node the tests run ok:

[oldeman@build-node easybuild-framework]$ module load python/3.5.4
[oldeman@build-node easybuild-framework]$ export PYTHONPATH=$HOME/easybuild-framework:$HOME/easybuild-easyblock

[oldeman@build-node easybuild-framework]$ python -O -m test.framework.options filter
Filtered CommandLineOptionsTest tests using 'filter', retained 2/91 tests: test_filter_deps, test_test_report_env_filter
..
----------------------------------------------------------------------
Ran 2 tests in 12.707s

OK
[oldeman@build-node easybuild-framework]$ which python
/cvmfs/soft.computecanada.ca/easybuild/software/2017/Core/python/3.5.4/bin/python
[oldeman@build-node easybuild-framework]$ module load python/3.6.3

The following have been reloaded with a version change:
  1) python/3.5.4 => python/3.6.3

[oldeman@build-node easybuild-framework]$ python -O -m test.framework.options filter
Filtered CommandLineOptionsTest tests using 'filter', retained 2/91 tests: test_filter_deps, test_test_report_env_filter
..
----------------------------------------------------------------------
Ran 2 tests in 13.263s

OK
[oldeman@build-node easybuild-framework]$ module load python/3.7.4

The following have been reloaded with a version change:
  1) python/3.6.3 => python/3.7.4

[oldeman@build-node easybuild-framework]$ python -O -m test.framework.options filter
Filtered CommandLineOptionsTest tests using 'filter', retained 2/91 tests: test_filter_deps, test_test_report_env_filter
..
----------------------------------------------------------------------
Ran 2 tests in 12.938s

OK

Let's hope travis is happy with them too.
(EB 4.0 dropped the vsc dependency so it now runs with our plain Python modules, except for some optional stuff).

@bartoldeman
Copy link
Copy Markdown
Contributor

bartoldeman commented Sep 8, 2019

Trying once more...

@bartoldeman bartoldeman closed this Sep 8, 2019
@bartoldeman bartoldeman reopened this Sep 8, 2019
bartoldeman
bartoldeman approved these changes Sep 9, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@bartoldeman bartoldeman left a comment

Choose a reason for hiding this comment

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

lgtm

@bartoldeman bartoldeman merged commit 047ef0f into easybuilders:develop Sep 9, 2019
@easybuilders easybuilders deleted a comment from boegelbot Sep 9, 2019
@easybuilders easybuilders deleted a comment from boegelbot Sep 9, 2019
@easybuilders easybuilders deleted a comment from boegelbot Sep 9, 2019
@easybuilders easybuilders deleted a comment from boegelbot Sep 9, 2019
@boegel
Copy link
Copy Markdown
Member

boegel commented Sep 9, 2019

@bartoldeman If a test frequently fails for no good reason with Python 3.5, we should try and get to the bottom of it, not re-trigger the tests until it passes. ;)

Is this the same test that was failing for your PR #2994, i.e. test_toy_build_formatv2 in https://travis-ci.org/easybuilders/easybuild-framework/jobs/581824226?

@mboisson mboisson deleted the filter_deps_fix branch May 6, 2020 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bartoldeman bartoldeman bartoldeman approved these changes

+1 more reviewer

@houndci-bot houndci-bot houndci-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@mboisson @bartoldeman @boegel @houndci-bot