update MotionCor2 easyblock to consider to locations for built binary by stefan-wolfsheimer · Pull Request #2541 · easybuilders/easybuild-easyblocks

stefan-wolfsheimer · 2021-08-12T16:54:56Z

No description provided.

stefan-wolfsheimer · 2021-08-12T16:56:52Z

see also:
easybuilders/easybuild-easyconfigs#13716

boegel · 2021-08-13T08:15:54Z

+    @staticmethod
+    def extra_options(extra_vars=None):
+        extra = {
+            'skip_rpath_check': [False, "Skip rpath check", None],


I would clarify the help string a bit here, and maybe also rename to skip_rpath_sanity_check

"Skip RPATH sanity check"

@stefan-wolfsheimer Can you clarify why you need this option, in which context?

boegel · 2021-08-13T08:17:14Z

            else:
                raise EasyBuildError(
-                    "Found multiple, or no, matching MotionCor2 binary named %s*" % self.motioncor2_bin
+                    "Found multiple, or no, matching MotionCor2 binary named %s" % pattern


@stefan-wolfsheimer This is clearly wrong, since there is no variable named pattern....
You probably still want to use self.motioncor2_bin like it was before (and leave the * in place too)?

boegel · 2021-08-13T08:18:20Z

        if (LooseVersion(self.version) >= LooseVersion("1.4")):
-            matches = glob.glob(os.path.join(self.builddir, '%s*' % self.motioncor2_bin))
+            pattern1 = os.path.join(self.builddir, '%s*' % self.motioncor2_bin)
+            pattern2 = os.path.join(self.builddir,


Maybe this deserves a comment: it seems like you're expecting to find the binary either directly in the build directory, or in a subdirectory named <name>_<version>?

Do you have any idea in which case it's in a subdirectory exactly?

I just tried to build MotionCor2 and the binary is in a subdirectory in the zipball distributed on https://emcore.ucsf.edu/ucsf-software. As far as I can tell this has changed since v1.4.0, for older version the binary was not in a subdirectory.

1.4.2 doesn't have a subdir so this needs more specified version checking. Or does that handle both at once?

hum right... However 1.4.0 does have a subdirectory.
Then I would say @stefan-wolfsheimer was right checking both directly in the build directory and in the subdirectory.

boegel · 2021-08-13T08:21:40Z

+            if not os.path.isfile(fp):
+                raise EasyBuildError("File not installed {}".format(fp))
+        if not self.cfg.get('skip_rpath_check', False):
+            super(EB_MotionCor2, self).sanity_check_step(custom_paths)


There's an easier way to only skip the RPATH sanity check:

def sanity_check_rpath(self, *args, **kwargs): """Custom implementation of RPATH sanity check: allow skipping via skip_rpath_sanity_check.""" if self.cfg.get('skip_rpath_sanity_check', False): self.log.info("Skipping RPATH sanity check, as specified via 'skip_rpath_sanity_check'...") else: super(EB_MotionCor2, self).sanity_check_rpath(*args, **kwargs)

Then you don't have to partially replicate the sanity check for files yourself either...

And maybe we should make skip_rpath_sanity_check a general easyconfig parameter in framework instead?

akesandgren · 2021-09-22T13:18:17Z

        if (LooseVersion(self.version) >= LooseVersion("1.4")):
-            matches = glob.glob(os.path.join(self.builddir, '%s*' % self.motioncor2_bin))
+            pattern1 = os.path.join(self.builddir, '%s*' % self.motioncor2_bin)
+            pattern2 = os.path.join(self.builddir,


1.4.2 doesn't have a subdir so this needs more specified version checking. Or does that handle both at once?

akesandgren · 2021-09-22T13:23:12Z


        if (LooseVersion(self.version) >= LooseVersion("1.4")):
-            matches = glob.glob(os.path.join(self.builddir, '%s*' % self.motioncor2_bin))
+            matches = glob.glob(os.path.join(self.builddir, '%s_%s' % (self.name, self.version), '%s*' % self.motioncor2_bin))


This is now wrong for 1.4.2

akesandgren · 2021-09-23T06:32:49Z

            else:
                raise EasyBuildError(
-                    "Found multiple, or no, matching MotionCor2 binary named %s*" % self.motioncor2_bin
+                    "Found multiple, or no, matching MotionCor2 binary named %s or %s" % (pattern1, patter2)


And once again, please do not change this.

MaximeMoge · 2021-09-23T09:49:26Z

I hope the latest commit answers all comments :)

I added a comment to clarify why we look for the binary both directly in the builddir and in a subdirectory.
I reverted the unnecessary change pointed out by akesandgren

akesandgren · 2021-09-23T11:39:19Z

        super(EB_MotionCor2, self).sanity_check_step(custom_paths)
+
+    def sanity_check_rpath(self, *args, **kwargs):
+        """Custom implementation of RPATH sanity check: allow skipping via skip_rpath_sanity_check."""


Please add comment on why we need to skip the rpath check

Actually looking at this again I think this is a workaround for a broader issue with RPATH for binary installs, and it is not the right approach:

we use RPATH on our supercomputer at SURF, and we usually fix the issues with binary install by customizing the easyconfig (or adding hooks) to run patchelf commands to add the missing rpaths in the binary.

this is an issue for all binary installs. Ideally, the rpath check in the case of binary install would also automatically fix the missing rpaths using patchelf. However patchelf cannot be used in the easyblock as it is not necessarily available on the system. So we have to do it at the easyconfig level.

In the case of the current PR, this custom sanity_check_rpath is in my opinion not the right solution. In fact this is a binary install, and the EB_MotionCor2 class should derive from the PackedBinary class instead of directly from the EasyBlock class. Since PackedBinary skips the rpath check, that would remove the need for a custom sanity_check_rpath.

A real fix for binary installs using RPATH should in my opinion be implemented in the Binary class rather than in software specific easyblocks, so we should not change the sanity_check_rpath at this level.

@akesandgren: my suggestion is to make EB_MotionCor2 derive from PackedBinary instead of EasyBlock, and remove the custom sanity_check_rpath completely. Does that sound good to you?

Not sure, I need to check what PackedBinary does. Probably won't have time the coming days/week

Lets consider that in a separate PR.

akesandgren · 2021-09-30T09:33:37Z

Test report by @akesandgren

Overview of tested easyconfigs (in order)

SUCCESS MotionCor2-1.2.6-GCCcore-8.2.0.eb
SUCCESS MotionCor2-1.3.2-GCCcore-8.3.0.eb
SUCCESS MotionCor2-1.3.1-GCCcore-8.3.0.eb
SUCCESS MotionCor2-1.4.2-GCCcore-10.2.0.eb
SUCCESS MotionCor2-1.4.4-GCCcore-10.3.0.eb

Build succeeded for 5 out of 5 (5 easyconfigs in total)
b-an02.hpc2n.umu.se - Linux Ubuntu 20.04, x86_64, Intel(R) Xeon(R) CPU E5-2690 v4 @ 2.60GHz, Python 3.8.10
See https://gist.github.com/ec2f81b499e93a103361b656bbec6fef for a full test report.

casparvl · 2021-09-30T09:37:46Z

@akesandgren which MotionCor2-1.4.4-GCCcore-10.3.0.eb did you use? Because the one from easybuilders/easybuild-easyconfigs#13716 still has CUDAcore, which should be replaced with CUDA? Or did you just make that change manually in order to be able to test the block?

In fact, I agree with Maxime Moge that it may make more sense to derive from PackedBinary. There is actually a more fundamental question how to deal with an installation if EASYBUILD_RPATH is set in case of binary installs that might also be interesting to @boegel and @ocaisa from an EESSI point of view. Should the necessary libraries be RPATHed in using something like patchelf? Should they not be RPATHed and the RPATH step be skipped? The latter is what PackedBinary does now. Regardless of what we decide, I think we should implement that behaviour in PackedBinary (and friends).

Anyway, having MotionCor2 derive from PackedBinary, we at least get any solution we decide upon later :)

Code changed.

akesandgren · 2021-09-30T09:39:10Z

I used my own which is a copy of the old 1.4.2 but with the required changes for use in 10.3.0 toolchain and deps updated accordingly. I.e. one that does not explicitly call unzip -j in the source section.

akesandgren · 2021-09-30T10:34:17Z

@casparvl Yes, using PackedBinary is probably a good idea, but as I said before, lets leave that to a separate PR.

akesandgren · 2021-09-30T10:35:32Z

Going in, thanks @stefan-wolfsheimer!

Partial revert of easybuilders#2541: Do not modify `cfg['configopts']` but simply read them and pass the combined result to `run_cmd`. This fixes an issue with templating while iterating over e.g. multiple Pythons and also avoids keeping values in configopts over multiple iterations/builds. Fixes easybuilders#2881

preparation for MotionCor2 1.4.4

ec9cd89

stefan-wolfsheimer mentioned this pull request Aug 12, 2021

{bio}[GCCcore/10.3.0] MotionCor2 v1.4.4 easybuilders/easybuild-easyconfigs#13716

Merged

boegel previously requested changes Aug 13, 2021

View reviewed changes

boegel added enhancement update labels Aug 13, 2021

boegel added this to the 4.x milestone Aug 13, 2021

akesandgren requested changes Sep 22, 2021

View reviewed changes

adjusted search pattern for >1.4

9a49414

akesandgren requested changes Sep 22, 2021

View reviewed changes

Stefan Wolfsheimer and others added 2 commits September 22, 2021 15:45

reverted to multiple search pattern

013e16e

Update motioncor2.py

d0d776d

akesandgren reviewed Sep 23, 2021

View reviewed changes

adding comment and fixing code style

f4a052f

akesandgren approved these changes Sep 23, 2021

View reviewed changes

akesandgren merged commit 0eb903a into easybuilders:develop Sep 30, 2021

akesandgren modified the milestones: 4.x, next release (4.5.0?) Sep 30, 2021

akesandgren mentioned this pull request Oct 4, 2021

{bio}[GCCcore/10.2.0] MotionCor2 v1.4.4 easybuilders/easybuild-easyconfigs#14074

Merged

boegel changed the title ~~preparation for MotionCor2 1.4.4~~ update MotionCor2 easyblock to consider to locations for built binary Oct 13, 2021

boegel mentioned this pull request Oct 13, 2021

remove custom sanity_check_rpath in MotionCor2 easyblock (doesn't work as intended) #2598

Merged

MaximeMoge mentioned this pull request Oct 25, 2021

derive custom easyblock from MotionCor2 from PackedBinary instead of EasyBlock #2607

Merged

Flamefire mentioned this pull request Feb 9, 2023

Fix CMake configopts when iterating (multiple builds) #2885

Closed

Conversation

stefan-wolfsheimer commented Aug 12, 2021

Uh oh!

stefan-wolfsheimer commented Aug 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akesandgren Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akesandgren Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaximeMoge commented Sep 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akesandgren commented Sep 30, 2021

Overview of tested easyconfigs (in order)

Uh oh!

casparvl commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akesandgren commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akesandgren commented Sep 30, 2021

Uh oh!

akesandgren commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

akesandgren Sep 22, 2021 •

edited

Loading

akesandgren Sep 22, 2021 •

edited

Loading

casparvl commented Sep 30, 2021 •

edited

Loading

akesandgren commented Sep 30, 2021 •

edited

Loading