Skip to content

flecsi: fixed reported issues in package#24398

Merged
alalazo merged 2 commits intospack:developfrom
alalazo:packages/flecsi_fix
Jun 26, 2021
Merged

flecsi: fixed reported issues in package#24398
alalazo merged 2 commits intospack:developfrom
alalazo:packages/flecsi_fix

Conversation

@alalazo
Copy link
Copy Markdown
Member

@alalazo alalazo commented Jun 18, 2021

$ spack audit packages flecsi
PKG-DIRECTIVES: 2 issues found
1. flecsi: wrong variant in "depends_on" directive
    invalid values for variant "backend" in package "flecsi": ['hpx@:1.9']
    in /home/culpo/PycharmProjects/spack/var/spack/repos/builtin/packages/flecsi/package.py
2. flecsi: wrong variant used for a dependency in a 'depends_on' directive
    the variant 'mpi' does not exist in package 'legion'
    in /home/culpo/PycharmProjects/spack/var/spack/repos/builtin/packages/flecsi/package.py

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jun 18, 2021

@ktsai7 @rspavel

depends_on('mpi', when='backend=legion @:1.9')
depends_on('mpi', when='backend=hpx @:1.9')
depends_on('legion+shared+mpi', when='backend=legion @:1.9')
depends_on('legion+shared network=mpi', when='backend=legion @:1.9')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note ever since 24c87e0 the Legion config that used to be spelled legion+mpi is now spelled legion network=gasnet conduit=mpi.

What you've asked for here is Legion's beta direct-to-MPI backend, which the Legion devs label as: "still experimental and we discourge its use for general (not legion development) use cases."

Are you really sure this is what you want?

You may get more stable behavior from the GASNet backend, and would almost certainly get better performance by enabling a native GASNet conduit (eg legion network=gasnet conduit=ibv for an InfiniBand system or legion network=gasnet conduit=aries on a Cray XC)

For more details, see the Legion package readme: https://github.com/spack/spack/tree/develop/var/spack/repos/builtin/packages/legion

CC: @pmccormick @streichler

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Are you really sure this is what you want?

Thanks for the insight. I am just trying out a command:

$ spack audit packages flecsi

that has been merged recently and that pointed to the 2 errors in the description. Translating +mpi to network=mpi was just a guess waiting to be validated by maintainers. By the way, I'm open to add new maintainers for the package if you or other people you think may be appropriate would like to volunteer 🙂

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@bonachea Given what you say above I wonder if:

depends_on('legion+shared', when='backend=legion @:1.9')
conflicts('legion network=none', when='backend=legion @:1.9')

is a more appropriate fix since it permits to concretize both with network=gasnet and network=mpi.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification.

Based on your response I'd suggest legion network=gasnet conduit=mpi as a more accurate equivalent to past behavior, although probably not the optimal choice for any distributed-memory system.

I'm definitely the wrong person to be marked as a maintainer for flecsi.

@rspavel @ktsai7 are marked as the current maintainers and hopefully have some thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

network=none is the already the Legion package default, which seems like a safe choice that the end user can override as appropriate for their system.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch.

I'll poke some of the flecsi/llegion folk when I get a chance, but I think the

depends_on('legion+shared', when='backend=legion @:1.9')
conflicts('legion network=none', when='backend=legion @:1.9')

Is probably the way to go for now as network and conduit are likely things that would need to be set on a per build basis.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@bonachea @rspavel Modified according to the discussion above. I checked and we can concretize both with legion network=mpi and legion network=gasnet.

@eugeneswalker
Copy link
Copy Markdown
Contributor

Thanks for working on this. I ran into this today.

@alalazo alalazo force-pushed the packages/flecsi_fix branch from efeeb25 to a5fc710 Compare June 23, 2021 06:40
@alalazo alalazo closed this Jun 23, 2021
@alalazo alalazo reopened this Jun 23, 2021
@alalazo alalazo mentioned this pull request Jun 23, 2021
@alalazo alalazo force-pushed the packages/flecsi_fix branch from 645d9fe to a5fc710 Compare June 23, 2021 07:56
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jun 23, 2021

CI is failing due to unrelated issues, see #23212. Once that is fixed this PR will be ready for another review.

alalazo added 2 commits June 23, 2021 14:16
This only prevents to use "legion network=none" when
flecsi has "backend=legion"
@alalazo alalazo force-pushed the packages/flecsi_fix branch from a5fc710 to fa632cc Compare June 23, 2021 12:17
@alalazo alalazo merged commit 17f9ddb into spack:develop Jun 26, 2021
@alalazo alalazo deleted the packages/flecsi_fix branch June 26, 2021 07:17
bollig pushed a commit to bollig/spack that referenced this pull request Jun 29, 2021
Prevent the use of "legion network=none" when
flecsi has "backend=legion"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants