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

add support for easystack file that contains easyconfig filenames + implement parsing of configuration options #4021

Merged
merged 23 commits into from
Oct 12, 2022

Conversation

boegel
Copy link
Member

@boegel boegel commented Jun 7, 2022

No description provided.

@boegel boegel added enhancement easystack Issues and PRs related to easystack files labels Jun 7, 2022
@boegel boegel modified the milestones: 4.x, next release (4.5.6?) Jun 7, 2022
@casparvl
Copy link
Contributor

casparvl commented Jun 8, 2022

Let me see if I get this right: essentially, you want to support an additional yaml structure where we don't specify the packages to be installed using e.g.

# easystack.yaml
software:
  R-bundle-Bioconductor:
    toolchains:
      foss-2020a:
        versions:
          '3.11':
            versionsuffix: '-R-4.0.0'
  GROMACS:
    toolchains:
      foss-2020a:
        versions:
          '2020.1':
            versionsuffix: '-Python-3.8.2'
          '2020.4':
            versionsuffix: '-Python-3.8.2'

You would specify

# easystack.yaml
easyconfigs:
  - R-bundle-Bioconductor-3.11-foss-2020a-R-4.0.0
  - GROMACS-2020.1-foss-2020a-Python-3.8.2
  - GROMACS-2020.4-foss-2020a-Python-3.8.2

Correct?

Some thoughts:

It's a lot more compact, but one of the main reasons we opted for the first (hierarchical) structure, is that you probably want to add certain options at the software level (e.g. for all GROMACS installations, use some --include-easyblocks-from-pr or something). In the second example, you'd have to specify that twice, remove two items if the easyblock gets merged, etc. I'm fine in adding this as a second format, I mean, we're still able to use the first format if whenever that is more suitable. However if we add a 3rd, 4th and 5th later on, things will get quite confusing. So I'm curious: what was your main reason for wanting this format?

Another question that came to mind: how will we specify options here? I'm no YAML guru and don't know what's allowed/possible here, but can we do e.g.

# easystack.yaml
easyconfigs:
  - R-bundle-Bioconductor-3.11-foss-2020a-R-4.0.0
  - GROMACS-2020.1-foss-2020a-Python-3.8.2:
    from_pr: 1234
  - GROMACS-2020.4-foss-2020a-Python-3.8.2

or something? Or do you then envision something more like:

# easystack.yaml
easyconfigs:
  - R-bundle-Bioconductor-3.11-foss-2020a-R-4.0.0
  - GROMACS-2020.1-foss-2020a-Python-3.8.2 --from-pr 1234
  - GROMACS-2020.4-foss-2020a-Python-3.8.2

The second is easier to write, but harder to parse

@casparvl
Copy link
Contributor

casparvl commented Jun 8, 2022

Just did a local test with

#easystack.yaml
robot: true
easyconfigs:
  - binutils-2.25-GCCcore-4.9.3
  - foss-2018a
  - R-bundle-Bioconductor-3.11-foss-2020a-R-4.0.0
  - GROMACS-2020.4-foss-2020a-Python-3.8.2

Running

eb --easystack easystack.yaml -D

That works fine. Mixing easyconfigs and software keywords silently ignores anything under software though:

$ cat easystack_format_mixed.yaml
robot: true
easyconfigs:
  - binutils-2.25-GCCcore-4.9.3
  - foss-2018a
  - R-bundle-Bioconductor-3.11-foss-2020a-R-4.0.0
  - GROMACS-2020.4-foss-2020a-Python-3.8.2
software:
  OpenFOAM:
    toolchains:
      foss-2020a:
        versions: ['8', 'v2006']
  R:
    toolchains:
      foss-2020a:
        versions: ['4.0.0']
$ eb --easystack easystack_format_mixed.yaml -D | grep FOAM
$ eb --easystack easystack_format_mixed.yaml -D | grep GROMACS
 * [ ] /home/casparl/.local/easybuild/Debian10/2021/software/EasyBuild/4.5.4-dev-boegel-easystack_easyconfigs/easybuild/easyconfigs/g/GROMACS/GROMACS-2020.4-foss-2020a-Python-3.8.2.eb (module: GROMACS/2020.4-foss-2020a-Python-3.8.2)

Copy link
Contributor

@casparvl casparvl left a comment

Choose a reason for hiding this comment

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

Looks reasonable overall, but lets discuss the individual items I touched upon :)

easybuild/framework/easystack.py Outdated Show resolved Hide resolved
easybuild/framework/easystack.py Outdated Show resolved Hide resolved
easybuild/framework/easystack.py Show resolved Hide resolved
easybuild/framework/easystack.py Outdated Show resolved Hide resolved
@boegel
Copy link
Member Author

boegel commented Jun 13, 2022

@casparvl

So I'm curious: what was your main reason for wanting this format?

Mainly because the current format is overly verbose for the common case where there's nothing special going on (no specific configuration options needed), etc.
I can probably rework this PR so it works with a "mixed" easystack, that uses both easyconfigs and software as top-level keys.

Another question that came to mind: how will we specify options here?

Good question, I didn't think that through yet. I think something like this could work:

easyconfigs:
    example-1.2.3:
        options: --from-pr 123
    example-4.5.6

I'll see if I can add dummy support for options, so we pick up on it, but don't do anything with it yet...

@casparvl
Copy link
Contributor

casparvl commented Jul 29, 2022

I've implemented the dummy support for option passing, will push in a second. The support that I've added is that the parser can now understand this format:

robot: true
easyconfigs:
  - pkgconf-1.8.0-GCCcore-11.3.0.eb:
      options: {
        'debug': True,
      }
  - ncurses-6.3-GCCcore-11.3.0.eb

i.e. easyconfigs is a list, but the items maybe be simple strings (and EasyConfig name), but may also be dictionaries. In case of a dictionary, it has this (raw) structure:

{'pkgconf-1.8.0-GCCcore-11.3.0.eb': None, 'options': {'debug': True}}

We can later quite easily have the parser store that in a data structure that satisfies the format proposed here for opts_per_ec: i.e. a dictionary, in which the keys are the easyconfig names, and the values are dictionaries with all the options.

@boegel boegel changed the title add support for easystack file that contains easyconfig filenames add support for easystack file that contains easyconfig filenames + implement parsing of configuration options Aug 3, 2022
…s neededso that the keys in the option dictionary can be expected to be the full EasyConfig names
…ndex the yaml dict _with_ the eb suffix, even though that dict contains the names _without_ suffix
ocaisa
ocaisa previously requested changes Aug 8, 2022
easybuild/framework/easystack.py Outdated Show resolved Hide resolved
easybuild/framework/easystack.py Show resolved Hide resolved
easybuild/framework/easystack.py Outdated Show resolved Hide resolved
msg += "Instead found keys: %s" % dict_keys
raise EasyBuildError(msg)

return easystack
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we verify that

  • The easyconfigs can actually be found
  • The options provided for each (if any) are valid

Copy link
Member

@ocaisa ocaisa Aug 8, 2022

Choose a reason for hiding this comment

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

We could consider resolving these with the robot and returning the full path to the ecs (and outputting these in debug mode).

We could also consider an option top prefer easyconfig files that are stored in the same path as the easystack file (which may not be in the robot path)...but I think that is a question of taste

Copy link
Member Author

Choose a reason for hiding this comment

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

The job of the easystack file is to just provide a list of easyconfigs, nothing more. It basically replaces listing easyconfig filenames in the eb command.

Checking whether the easyconfigs can actually be found is done later through main (via det_easyconfig_paths).

Likewise for the options: passing those to the option parser, which will check if the options are valid, is done later (that's a part of @casparvl's PR #4057)

…nment' that one would get if the easystack file already included .eb extensions
@boegel boegel modified the milestones: 4.6.1, release after 4.6.1 Sep 9, 2022
@boegel
Copy link
Member Author

boegel commented Oct 12, 2022

Documentation will be updated to mention support for top-level easyconfigs key when #4057 is also merged (which adds support for actually applying the configuration options specific to a particular easystack entry)


return easyconfig_names, general_options
_log.debug("Using EasyConfig specific options based on the following dict:")
_log.debug(easystack.ec_opts)
Copy link
Member Author

Choose a reason for hiding this comment

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

We could use pprint.pformat here to make it more readable (let's do that in #4057)

@@ -266,7 +265,7 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):
# TODO add general_options (i.e. robot) to build options
orig_paths, general_options = parse_easystack(options.easystack)
Copy link
Member Author

Choose a reason for hiding this comment

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

general_options is not correct here, but this is being fixed in #4057 (renamed to opts_per_ec)

@casparvl casparvl dismissed ocaisa’s stale review October 12, 2022 12:48

Review has been processed

Copy link
Contributor

@casparvl casparvl left a comment

Choose a reason for hiding this comment

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

Went through this together with @boegel and looks good

Any polishing we still need to do can be done in #4057

@casparvl casparvl merged commit 54655d3 into easybuilders:develop Oct 12, 2022
@boegel boegel deleted the easystack_easyconfigs branch October 12, 2022 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easystack Issues and PRs related to easystack files enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants