@boegel I'm using eb --from-pr 9338 --upload-test-report --rebuild --installpath /tmp/$USER but it seems to use the EC in my local git repo instead of from this PR. Any idea why?

@Flamefire Hmm, --from-pr is supposed to hard inject the path to easyconfigs from the PR...

Can you share the output of eb --show-config?

@Flamefire We'll have to pick between #9308 (which is now require to go imho) or this one...

Can you share the output of eb --show-config?

Sure:

#
# Current EasyBuild configuration
# (C: command line argument, D: default value, E: environment variable, F: configuration file)
#
allow-loaded-modules  (F) = EasyBuild, GC3Pie, ccache
buildpath             (F) = /scratch/ws/s3248973-EasyBuild/easybuild_build
containerpath         (D) = /home/s3248973/.local/easybuild/containers
detect-loaded-modules (F) = unload
github-user           (F) = Flamefire
installpath           (F) = /scratch/ws/s3248973-EasyBuild/easybuild
job-backend-config    (F) = /home/s3248973/.gc3/gc3pie.conf
repositorypath        (D) = /home/s3248973/.local/easybuild/ebfiles_repo
robot-paths           (F) = /home/s3248973/git/easybuild-easyconfigs/easybuild/easyconfigs, /projects/hpcsupport/easybuild/easyconfigs-ZIH
sourcepath            (D) = /home/s3248973/.local/easybuild/sources
subdir-software       (F) = eb
tmp-logdir            (F) = /tmp/easybuild-tmplog
tmpdir                (F) = /tmp/easybuild-tmp

We'll have to pick between #9308 (which is now require to go imho) or this one...

I guess both are (at least IMO). Although I'm a bit confused, that sanity_pip_check doesn't need to go into exts_default_options. I created versions and order of this PR by using https://gist.github.com/Flamefire/49426e502cd8983757bd01a08a10ae0d so this one might be more recent and "more correct". Feel free to compare those and pick what you think would be best if both "work"

@Flamefire W.r.t. sanity_pip_check: all easyconfig parameters set for the "parent" are inherited by the extensions, so both enabling sanity_pip_check "directly" and via exts_default_options actually works.
Some stuff (like source_tmpl) can only be specified for extensions though, and require the use of exts_default_options. A lot of this has grown historically...

W.r.t. both PRs: this one doesn't have a successful test report yet, it uses a patch for astor that can be avoided by using a slightly older setuptools with Python 3.7.2 (which is not needed for Python 3.7.4 which includes setuptools 41.2.0), and it shuffles things more than is actually needed.
Also, you're removing TensorFlow-1.13.1_lrt-flag.patch from TensorFlow-2.0.0-foss-2019a-Python-3.7.2.eb while there's no need to afaik.

So, my vote currently goes to #9308, but of course I'm a bit biased. ;)

If you're OK with going forward with #9308, do you mind submitting a test report for it?

this one doesn't have a successful test report yet

I tried but EB failed me :(

it uses a patch for astor that can be avoided by using a slightly older setuptools

Sure. That was my initial comment, but as ML guys usually want the latest and greatest...

it shuffles things more than is actually needed

Well it actually follows a defined workflow to order the packages which can be verified automatically. I consider this a plus

Also, you're removing TensorFlow-1.13.1_lrt-flag.patch

Both are TF 2.0 so I figured it makes sense to have both ECs be (almost) the same.

If you're OK with going forward with #9308, do you mind submitting a test report for it?

I can for both if you can help fix the EB issue I'm experiencing. I'd consider this a (serious) bug if it tests something else then it should. But this might probably sometime next week.

it uses a patch for astor that can be avoided by using a slightly older setuptools

Sure. That was my initial comment, but as ML guys usually want the latest and greatest...

That's true in general, but in the case of setuptools it really doesn't matter.

Also, you're removing TensorFlow-1.13.1_lrt-flag.patch

Both are TF 2.0 so I figured it makes sense to have both ECs be (almost) the same.

OK, but then we should add the patch where it's missing, not drop it (it's there for a reason, see #6089

If you're OK with going forward with #9308, do you mind submitting a test report for it?

I can for both if you can help fix the EB issue I'm experiencing. I'd consider this a (serious) bug if it tests something else then it should. But this might probably sometime next week.

You're right that it seems fishy, but it's not clear to me how you deduced that it's using the wrong easyconfig file?

The upload log in the test report is not a debug log, so it's hard to tell what went wrong. Maybe try again with eb --debug --from-pr 9338 ...?

Also, try this: eb --debug --from-pr 9338 --dry-run, and share the output. Which paths does it print for the TensorFlow-2.0.0 easyconfigs? They should be in a temporary directory, something like:

 /tmp/eb-LJ74Bd/files_pr9338/t/TensorFlow/TensorFlow-2.0.0-foss-2019a-Python-3.7.2.eb

That's true in general, but in the case of setuptools it really doesn't matter.

Ok, then I can revert that.

OK, but then we should add the patch where it's missing, not drop it (it's there for a reason, see #6089

Quoting you from #9254 (comment): the -lrt patch is only required for old operating systems like CentOS 6 (see #6259 for example), I think it's OK to just drop it for TensorFlow 2.0.0.

I don't mind readding this but to me it seemed to not be required anymore.

You're right that it seems fishy, but it's not clear to me how you deduced that it's using the wrong easyconfig file?

Because it is using a patch file from my repo: Checksum for extension patch /home/s3248973/git/easybuild-easyconfigs/easybuild/easyconfigs/t/TensorFlow/TensorFlow-1.14.0_fix-cuda-build.patch failed, see https://gist.github.com/Flamefire/8bc53f28d963be28b4ce677b85e64995

BTW: I can no longer test ECs with EB 4.0.1 because the Java EC uses a new version format the old EB cannot handle. Does EB use semantic versioning? If so, the next version should be 5.x

Quoting you from #9254 (comment): the -lrt patch is only required for old operating systems like CentOS 6 (see #6259 for example), I think it's OK to just drop it for TensorFlow 2.0.0.

I don't mind readding this but to me it seemed to not be required anymore.

I was under the impression then that the existing patch didn't work anymore for TensorFlow 2.0.0, but it does work, so it's a no-brainer to keep using it...

You're right that it seems fishy, but it's not clear to me how you deduced that it's using the wrong easyconfig file?
Because it is using a patch file from my repo: Checksum for extension patch /home/s3248973/git/easybuild-easyconfigs/easybuild/easyconfigs/t/TensorFlow/TensorFlow-1.14.0_fix-cuda-build.patch failed, see https://gist.github.com/Flamefire/8bc53f28d963be28b4ce677b85e64995

That patch file is not touched in this PR, only files that are touched in this PR are downloaded from the PR.
Besides, there's a SHA256 checksum for the patch in the easyconfig file, which is checked during installation, so as long as that matches, it's fine if it picks up the patch file from another location.

BTW: I can no longer test ECs with EB 4.0.1 because the Java EC uses a new version format the old EB cannot handle. Does EB use semantic versioning? If so, the next version should be 5.x

You'll have to clarify this...
We haven't (deliberately/knowingly) broken anything that would warrant a major version bump for the next release.

Which Java easyconfig is troublesome (and how), using which EasyBuild version?

That patch file is not touched in this PR, only files that are touched in this PR are downloaded from the PR.

I missed that, thanks.

You'll have to clarify this...

The error message is caused by

Traceback (most recent call last):
  File "/usr/lib64/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/lib64/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/software/generic/EasyBuild/4.0.1/lib/python2.7/site-packages/easybuild/main.py", line 467, in <module>
    main()
  File "/software/generic/EasyBuild/4.0.1/lib/python2.7/site-packages/easybuild/main.py", line 395, in main
    txt = dry_run(easyconfigs, modtool, short=not options.dry_run)
  File "/sw/installed/EasyBuild/4.0.1/lib/python2.7/site-packages/easybuild/tools/robot.py", line 247, in dry_run
    all_specs = resolve_dependencies(easyconfigs, modtool, retain_all_deps=True, raise_error_missing_ecs=False)
  File "/sw/installed/EasyBuild/4.0.1/lib/python2.7/site-packages/easybuild/tools/robot.py", line 436, in resolve_dependencies
    processed_ecs = process_easyconfig(path, validate=not retain_all_deps, hidden=hidden)
  File "/sw/installed/EasyBuild/4.0.1/lib/python2.7/site-packages/easybuild/framework/easyconfig/easyconfig.py", line 1776, in process_easyconfig
    ec = EasyConfig(spec, build_specs=build_specs, validate=validate, hidden=hidden)
  File "/sw/installed/EasyBuild/4.0.1/lib/python2.7/site-packages/easybuild/framework/easyconfig/easyconfig.py", line 472, in __init__
    self.parse()
  File "/sw/installed/EasyBuild/4.0.1/lib/python2.7/site-packages/easybuild/framework/easyconfig/easyconfig.py", line 669, in parse
    self.generate_template_values()
  File "/sw/installed/EasyBuild/4.0.1/lib/python2.7/site-packages/easybuild/framework/easyconfig/easyconfig.py", line 1423, in generate_template_values
    self._generate_template_values()
  File "/sw/installed/EasyBuild/4.0.1/lib/python2.7/site-packages/easybuild/framework/easyconfig/easyconfig.py", line 1453, in _generate_template_values
    template_values = template_constant_dict(self, ignore=ignore)
  File "/sw/installed/EasyBuild/4.0.1/lib/python2.7/site-packages/easybuild/framework/easyconfig/templates.py", line 232, in template_constant_dict
    dep_version_parts = dep_version.split('.')
AttributeError: 'dict' object has no attribute 'split'

The error message is caused by

easybuild-easyconfigs/easybuild/easyconfigs/j/Java/Java-1.8.eb

Line 18 in d5d750c

('Java', {'arch=x86_64': '%(version)s.0_221',

which has the dependency version a dict, instead of a string. It seems support for this is in current EB, but EB 4.0.1 does not know how to handle this:

You shouldn't use easyconfigs from the develop branch unless you're also using the develop branch for both easybuild-framework and easybuild-easyblocks , use the easyconfigs that ship with EasyBuild instead (or the one from the master branch).

Support for specifying dependency versions as a dict was implemented in easybuilders/easybuild-framework#3047, in a backwards-compatible way.

You shouldn't use easyconfigs from the develop branch unless you're also using the develop branch for both easybuild-framework and easybuild-easyblocks , use the easyconfigs that ship with EasyBuild instead (or the one from the master branch).

This sounds strange. How am I supposed to test the new ECs if I have to install (how?) the latest dev of EB too? Shouldn't (unless a new major version of the EC repo is announced) the new ECs work with the current EB?

Support for specifying dependency versions as a dict was implemented in easybuilders/easybuild-framework#3047, in a backwards-compatible way.

Well while the next EB can read current and next ECs the current EB can only read current ECs. IMO this is a non-backwards-compatible change. At least for the ECs.

But well... Is there some guide on how to track new EB and EasyBlocks when using the new ECs? Something easy?

You shouldn't use easyconfigs from the develop branch unless you're also using the develop branch for both easybuild-framework and easybuild-easyblocks , use the easyconfigs that ship with EasyBuild instead (or the one from the master branch).

This sounds strange. How am I supposed to test the new ECs if I have to install (how?) the latest dev of EB too? Shouldn't (unless a new major version of the EC repo is announced) the new ECs work with the current EB?

In this particular case, it should work just fine if you use the latest EasyBuild v4.0.1 release + eb --from-pr (since then Java will be installed using the easyconfigs shipped with EasyBuild v4.0.1).

In general though, PRs should be tested using the develop branches of all repos, to make sure that everything that is needed is supported.
In this particular case, you can get away by downloading the enhanced PythonPackage easyblock from the develop branch (to pick up the pip check support added in easybuilders/easybuild-easyblocks#1853), and make EasyBuild v4.0.1 use it via eb --include-easyblocks pythonpackage.py (see also https://easybuild.readthedocs.io/en/latest/Including_additional_Python_modules.html#including-additional-easyblocks-include-easyblocks).

Support for specifying dependency versions as a dict was implemented in easybuilders/easybuild-framework#3047, in a backwards-compatible way.

Well while the next EB can read current and next ECs the current EB can only read current ECs. IMO this is a non-backwards-compatible change. At least for the ECs.

The changes in develop are backwards-compatible meaning an EasyBuild installation using that code can parse & use easyconfigs that worked with existing EasyBuild releases (except for breaking changes in major EasyBuild versions).

EasyBuild releases are not forward-compatible though: easyconfigs in develop may not work with existing releases, because they require new features (like the dependency-version-as-dict feature added to framework, or a new/enhanced easyblock, etc.)

But well... Is there some guide on how to track new EB and EasyBlocks when using the new ECs? Something easy?

Using the develop branches basically boils down to doing a git clone on all 3 easybuild-* repos, making sure the develop branch is checked out (and updated if needed), and adding the locations to $PYTHONPATH (+ the easybuild-framework location to $PATH for the eb command).

There's some scripts available to facilitate this, see https://easybuild.readthedocs.io/en/latest/Installation_Alternative.html#installation-of-latest-development-version .

In this particular case, it should work just fine if you use the latest EasyBuild v4.0.1 release + eb --from-pr (since then Java will be installed using the easyconfigs shipped with EasyBuild v4.0.1).

It wouldn't because I need all the GCCCore 8.3.0 dependencies that do not exist in current ECs. Or would they be downloaded too?

Test report by @Flamefire
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
taurusi6216.taurus.hrsk.tu-dresden.de - Linux RHEL 7.4, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/0b413886ca70c1da800587d3f8dec02a for a full test report.

In this particular case, it should work just fine if you use the latest EasyBuild v4.0.1 release + eb --from-pr (since then Java will be installed using the easyconfigs shipped with EasyBuild v4.0.1).

It wouldn't because I need all the GCCCore 8.3.0 dependencies that do not exist in current ECs. Or would they be downloaded too?

Good question... Only stuff touched in the PR is pulled in when using --from-pr, dependencies not included in the EasyBuild version you're using would be an issue.

So it boils down to: testing a PR with --from-pr may work with the latest EasyBuild release, but the safest bet is to use latest develop branches.

@Flamefire Now that we have successful test reports both here and in #9308, we'll have to pick what we go forward with.

For the reasons mentioned in #9338 (comment), my preference still goes to #9308 .

Is there anything included here that is not included in #9308 that is crucial?

Have you verified that the TensorFlow-1.14.0_fix-cuda-mpi.patch is not needed for 2.0?

Have you verified that the TensorFlow-1.14.0_fix-cuda-mpi.patch is not needed for 2.0?

It is required. You need to update the repo at /hpc2n/eb/custom/easyconfigs to include the latest version of it.

Is there anything included here that is not included in #9308 that is crucial?

I'll double check yours again, but this seems more complete. Yours e.g. is not installing pyasn1-modules-0.2.7, see my list at #9308 (comment) created by looking at pip install tensorflow

I'll double check yours again, but this seems more complete. Yours e.g. is not installing pyasn1-modules-0.2.7, see my list at #9308 (comment) created by looking at pip install tensorflow

pyasn1-modules is not actually needed, since pip check isn't complaining?

This will add quite some more modules and you'll probably arrive at the same as mine here ;-)

Re:

W.r.t. both PRs: this one doesn't have a successful test report yet, it uses a patch for astor that can be avoided by using a slightly older setuptools with Python 3.7.2 (which is not needed for Python 3.7.4 which includes setuptools 41.2.0), and it shuffles things more than is actually needed.
Also, you're removing TensorFlow-1.13.1_lrt-flag.patch from TensorFlow-2.0.0-foss-2019a-Python-3.7.2.eb while there's no need to afaik.

I added the lrt patch and removed the astor patch by downgrading setuptools. So IMO this is complete now. Only thing is the shuffeling but that is due to using a tool to determine the order

Edit: Ah, I'm using tensorboard 2.0.1 while you are using 2.0.0. That dependency is new

Ah, I should fix that...

This will add quite some more modules and you'll probably arrive at the same as mine here ;-)

Actually, I'd prefer not updating to tensorboard 2.0.0, especially not in the existing TensorFlow-2.0.0-foss-2019a-Python-3.7.2.eb which is included in the last EasyBuild release (we don't tweak versions in existing easyconfigs unless there's a good reason for it).
And it makes sense to keep TensorFlow-2.0.0-foss-2019a-Python-3.7.2.eb and TensorFlow-2.0.0-fosscuda-2019a-Python-3.7.2.eb in sync...

You mean "I'd prefer not updating to tensorboard 2.0.1," ??
Also tensorflow-estimator should probably be kept at 2.0.0

You mean "I'd prefer not updating to tensorboard 2.0.1," ??

Yes, sorry.

Also tensorflow-estimator should probably be kept at 2.0.0

I'm not touching that in #9308

@Flamefire I don't want to let your efforts go to waste, let me be clear on that.
But if we start updating extensions in existing easyconfigs, then it's a never-ending story...

No but this PR changes to 2.0.1 for both tensorboard and tensorflow-estimator

@Flamefire What I meant with my question about fix-cuda-mpi.patch is that it is not included in this PR

(The fix-cuda-build.patch I had has been removed now)

Actually, I'd prefer not updating to tensorboard 2.0.0, especially not in the existing TensorFlow-2.0.0-foss-2019a-Python-3.7.2.eb which is included in the last EasyBuild release (we don't tweak versions in existing easyconfigs unless there's a good reason for it).

with

And it makes sense to keep TensorFlow-2.0.0-foss-2019a-Python-3.7.2.eb and TensorFlow-2.0.0-fosscuda-2019a-Python-3.7.2.eb in sync...

and

But if we start updating extensions in existing easyconfigs, then it's a never-ending story...

IMO the currently released TF 2.0 EC is faulty and must be reinstalled due to missing dependencies. The ECS here were derived by a programmatic approach determining the current dependencies as installed by pip and their dependencies sorted by a deterministic order. So while I agree to not tweak versions in existing ECs IMO it is reasonable to do here especially to keep both TF 2.0 ECS in sync (because the existing EC should not be used). To be clear: This is not updating an existing EC to tweak versions, but a structured approach to fix a broken EC by relying on officially available dependencies (as reported by pip)

Also tensorflow-estimator should probably be kept at 2.0.0

IMO it should be updated according to the release notes: https://github.com/tensorflow/estimator/releases

TensorFlow Estimator 2.0 has been released with a private import of a symbol from TensorFlow master. However, that symbol does not exist on TensorFlow 2.0, due to a race condition regarding branch cuts.

This release patches TensorFlow Estimator to resolve this symbol not found error.

@akesandgren

What I meant with my question about fix-cuda-mpi.patch is that it is not included in this PR

Sorry?: https://github.com/easybuilders/easybuild-easyconfigs/pull/9338/files#diff-966a97db0e97e986124e9b34a2198830R124 Or do you mean for the foss? There it is not required because no CUDA

@Flamefire What I meant is that there is a TensorFlow-1.14.0_fix-cuda-mpi.patch that is no longer used in the TF 2.0 ECs. I was wondering if you had verified that it is no longer needed.

That release note for tensorflow-estimator makes it very clear that we need to update it. So I fully agree on that.

@Flamefire Thanks a lot for the clarification, the tensorflow-estimator issue should indeed be fixed.

And with the cleanups you did, this PR is in better shape than #9308, so let's go forward with this instead..
I'll get test reports submitted, just add the additional comments w.r.t. the custom setuptools...

W.r.t. https://gist.github.com/boegel/fd9a636d652aa5c8e57778088e9c0a21 and your enhanced version https://gist.github.com/Flamefire/49426e502cd8983757bd01a08a10ae0d, I think we should look into integrating that in easybuild-framework...
Are you up for helping out with that?

What I meant is that there is a TensorFlow-1.14.0_fix-cuda-mpi.patch that is no longer used in the TF 2.0 ECs. I was wondering if you had verified that it is no longer needed.

Ah, I had the wrong one. Yes that patch is in upstream now: https://github.com/tensorflow/tensorflow/blob/v2.0.0/tensorflow/contrib/mpi_collectives/kernels/ring.cu.cc#L23

Regarding the deep check thingie, should that be a standalone thing to be used before creating a new PR too?
And not just part of a framework check (if that is what @boegel was referring to)

Test report by @boegel
FAILED
Build succeeded for 1 out of 2 (2 easyconfigs in this PR)
generoso - Linux centos linux 7.6.1810, Intel(R) Xeon(R) CPU E5-2660 v4 @ 2.00GHz, Python 3.6.8
See https://gist.github.com/151b69833ab694387e7db3a667276924 for a full test report.

edit: ignore this failure, outdated patch...

Test report by @boegel
SUCCESS
Build succeeded for 3 out of 3 (2 easyconfigs in this PR)
node3309.joltik.os - Linux centos linux 7.7.1908, Intel(R) Xeon(R) Gold 6242 CPU @ 2.80GHz, Python 2.7.5
See https://gist.github.com/9b5be674b994b823f1351d78a14790f7 for a full test report.

@akesandgren The pip check support is part of PythonPackage now, see easybuilders/easybuild-easyblocks#1853.

The feature is about automatically constructing exts_list in the right order for bundles of Python packages, which can be done by installing the Python package to /tmp, and using pipdeptree to figure out the order of the (missing) extensions.

Test report by @boegel
FAILED
Build succeeded for 1 out of 2 (2 easyconfigs in this PR)
node2455.golett.os - Linux centos linux 7.7.1908, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/f80edf9b58e0434ed28e5a6137a0a0b1 for a full test report.

edit: failed due to missing extensions from #9329, fixing...

Test report by @akesandgren
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
b-an03.hpc2n.umu.se - Linux ubuntu 16.04, Intel(R) Xeon(R) CPU E5-2690 v4 @ 2.60GHz, Python 2.7.12
See https://gist.github.com/eda0793ded9f9400a35e14fdaaf93474 for a full test report.

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
node2455.golett.os - Linux centos linux 7.7.1908, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/45b276b101fc4f378c9c423a3cc357cc for a full test report.

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
node3111.skitty.os - Linux centos linux 7.7.1908, Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz, Python 3.6.8
See https://gist.github.com/f18016c0724385e2ab331bf3b41b7407 for a full test report.

Going in, thanks @Flamefire!

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
generoso-1 - Linux centos linux 7.6.1810, Intel(R) Xeon(R) CPU E5-2660 v4 @ 2.00GHz, Python 3.6.8
See https://gist.github.com/fdc60a67129e14521914267f94f27527 for a full test report.