Skip to content

[xSDK] PETSc review#1999

Closed
tgamblin wants to merge 2 commits intoxsdk/review-targetfrom
xsdk/reviews/petsc
Closed

[xSDK] PETSc review#1999
tgamblin wants to merge 2 commits intoxsdk/review-targetfrom
xsdk/reviews/petsc

Conversation

@tgamblin
Copy link
Copy Markdown
Member

This is a dummy pull request created so that developers of xSDK packages can review their package.py files in Spack. This file is already integrated in Spack, we're just making a PR so that it can be commented on.

See the package.py file in the mainline develop branch to see blame for particular lines of code. This can be informative, because you can see the commits the code came from and why it's there.

@tgamblin tgamblin added the xSDK label Oct 11, 2016
@tgamblin tgamblin changed the title XSDK PETSc review [xSDK] PETSc review Oct 11, 2016
@tgamblin
Copy link
Copy Markdown
Member Author

@BarrySmith

@BarrySmith
Copy link
Copy Markdown
Contributor

BarrySmith commented Oct 11, 2016

  1. PETSc doesn't depend on boost in any way so you can remove all the boost stuff.

  2. Having system specific things in these spack files like

if sys.platform != "darwin":
    compiler_opts.extend([
        '--with-cpp=cpp',
        '--with-cxxcpp=cpp',
    ])

seems undesirable. Is there something we should add to PETSc ./configure so you don't need this stuff?

  1. Won't it be better if petsc.py used the PETSc tests to check if the build was successful instead of having its own custom tests?. To run the PETSc tests you first need to set PETSC_DIR to the INSTALLED location (but remain in the build directory) unset PETSC_ARCH and then run
    make test. We can add something to the output like Failed or Succeeded if you want that.

  2. Why doesn't each package contain its spack xxxx.py file in its own repository instead of in the central spack repository? Then it would be the PETSc team's responsibility to manage it and keep it up to date.

@eschnett
Copy link
Copy Markdown
Contributor

eschnett commented Oct 11, 2016

@BarrySmith

Regarding (2): Yes, this would be nice. I don't know why PETSc prefers the system cpp instead of the first cpp found in PATH -- that what the options address. The system-specific issue here has to do with using Spack-built (i.e. relatively vanilla) GCC on macOS "El Capitan". The problem there is usually that Apple's header files contain Objective C code or preprocessor functionality that's not part of the C standard, hence GCC doesn't always work. Here, the solution happens to be to use the system cpp instead of GCC's cpp...

Regarding (4): If there's a problem with installing a package on a new OS release, or using a newer version of Clang or GCC, or with a new version of OpenSSL, then Spack can provide a new package within hours. Software releases are usually not that fast. It's the same case as for providing Ubuntu, Debian, Homebrew, and MacPorts packages.

However, Spack does have a scalability problem that your suggestion would address. The long-term solution is probably somewhere in the middle, maybe with PETSc providing a base package description, and some other mechanism being able to override it in emergencies.

@tgamblin
Copy link
Copy Markdown
Member Author

  1. PETSc doesn't depend on boost in any way so you can remove all the boost stuff.

@alalazo @davydden: you're credited with the boost arguments here -- does PETSc build ok if you don't include them? @BarrySmith says we don't need them (but then why does PETSc accept boost args?)

@tgamblin
Copy link
Copy Markdown
Member Author

  1. Why doesn't each package contain its spack xxxx.py file in its own repository instead of in the central spack repository? Then it would be the PETSc team's responsibility to manage it and keep it up to date.

Regarding (4): If there's a problem with installing a package on a new OS release, or using a newer version of Clang or GCC, or with a new version of OpenSSL, then Spack can provide a new package within hours. Software releases are usually not that fast. It's the same case as for providing Ubuntu, Debian, Homebrew, and MacPorts packages.

However, Spack does have a scalability problem that your suggestion would address. The long-term solution is probably somewhere in the middle, maybe with PETSc providing a base package description, and some other mechanism being able to override it in emergencies.

There are certainly use cases where having PETSc maintain a package.py would be useful but that doesn't allow us to curate the PETSc package in Spack as part of a larger distro -- if the PETSc developers introduce a bug, we can't fix it, as @eschnett points out. So I don't think making an external PETSc package.py the default is a good idea.

That said, Spack has external repositories (spack repo add) that can be used to overlay a custom PETSc package on the builtins, so you can override things. There was also a good discussion in #1136 of how we might allow building one-off packages directly from URLs, like some other package managers do. Then you could easily use Spack to install an external package.py without a lot of configuration.

@tgamblin
Copy link
Copy Markdown
Member Author

  1. Won't it be better if petsc.py used the PETSc tests to check if the build was successful instead of having its own custom tests?. To run the PETSc tests you first need to set PETSC_DIR to the INSTALLED location (but remain in the build directory) unset PETSC_ARCH and then run
    make test. We can add something to the output like Failed or Succeeded if you want that.

This is helpful and probably what we should do when we add longer, optional tests. I believe the test currently in there is supposed to be a simple smoke test. Running the actual PETSc tests would be even better.

@tgamblin
Copy link
Copy Markdown
Member Author

@BarrySmith: regarding (2), see #1835, where the related discussion with @eschnett is. Maybe this is a PETSc issue we could remove for future versions.

@pramodk
Copy link
Copy Markdown
Contributor

pramodk commented Oct 11, 2016

@BarrySmith : how to handle cross compiling build for petsc in spack?

@BarrySmith
Copy link
Copy Markdown
Contributor

Cross compiling is done using the argument --with-batch to ./configure. What happens with --with-batch is ./configure runs as usual except it does not attempt to RUN any compiled code instead it saves all the run tests into a single C file. When ./configure is done it tells the user to run a generated executable; (on a batch system you would submit the executable through the batch system, with straight cross compiling you would run the executable on that other system). The executable produces a new python script that the user than runs that completes the configure process. Then make and make install in the usual way. Try it. You don't need a batch system to use --with-batch you can run it with -with-batch on a laptop to get an idea how it works.

If you make spack able to "submit jobs" then you could easily utilize the --with-batch option. Otherwise you will have to do what we do and interupt spack to have the user run the executable and then restart spack with the new information.

@goxberry
Copy link
Copy Markdown
Contributor

@eschnett re: (2) Could this issue be solved by copying patched versions of the system headers into the gcc install, as in #1518? This solution seems like a cleaner design because all changes are localized to the gcc package instead of propagated through multiple packages by inserting new preprocessor options.

@davydden
Copy link
Copy Markdown
Member

The reason for (3) was to have quich tests of external solvers/preconditioners. This helped in the past to pick up broken MUMPS build on macOS.

If PETS has somewhere tests with, say, laplace equation with superlu-dist, mumps, hypre -- we can switch to running those as native make targets. Even more convenient would be to have those in default "make tests".

  1. Won't it be better if petsc.py used the PETSc tests to check if the build was successful instead of having its own custom tests?. To run the PETSc tests you first need to set PETSC_DIR to the INSTALLED location (but remain in the build directory) unset PETSC_ARCH and then run
    make test. We can add something to the output like Failed or Succeeded if you want that

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Oct 12, 2016

@alalazo @davydden: you're credited with the boost arguments here -- does PETSc build ok if you don't include them?

I am currently trying a build, but I would expect so from a variant.

@BarrySmith Looking at the log, it seems I added boost in f01d1c4 As I did for a lot of other software, I packaged it just looking at the configure (or other relevant build information), which in this case says for boost :

BOOST:
  --with-boost=<bool>
       Indicate if you wish to test for boost  current: 0
  --with-boost-dir=<dir>
       Indicate the root directory of the boost installation
  --with-boost-pkg-config=<dir>
       Look for boost using pkg-config utility optional directory to look in
  --with-boost-include=<dirs>
       Indicate the directory of the boost include files
  --with-boost-lib=<libraries: e.g. [/Users/..../libboost.a,...]>
       Indicate the boost libraries
  --download-boost=<no,yes,filename,url>
       Download and install boost  current: no
  --download-boost-commit=commitid
       The commit id from a git repository to use for the buildboost  current: 0

EDIT : build without boost was successfull on Ubuntu 14.04 + [email protected] + [email protected]

@tgamblin
Copy link
Copy Markdown
Member Author

@BarrySmith: is that stuff just so PETSc can download boost if it needs it for some dependency?

@citibeth
Copy link
Copy Markdown
Member

is that stuff just so PETSc can download boost if it needs it for some dependency?

Spack packages should not be downloading anything during installation.

That said, Spack has external repositories (spack repo add) that can be used to overlay a custom > PETSc package on the builtins, so you can override things.

You can also fork the GitHub repo. But I don't think either case really provides a scalable solution. Maybe one could have a scheme that depends on Git to merge 100 independently curated repos.

Spack should not be building OpenSSL if at all possible, so no need for Spack updates here. I doubt the typical Spack user has the information required to keep it up to date.

@tgamblin
Copy link
Copy Markdown
Member Author

Spack packages should not be downloading anything during installation.

Yes. But PETSc can act like a standalone installer (it has its own rather fancy build system that does this). So the question is really for @BarrySmith to clarify why the boost args are there, if PETSc doesn't depend on boost.

Which brings up another question for @BarrySmith: if we omit the boost args, are there cases where PETSc will then try to download boost? Seems like we might need to add --download-boost=no if we remove them.

@BarrySmith
Copy link
Copy Markdown
Contributor

BarrySmith commented Oct 12, 2016

Yes, PETSc has a variety of packages it can download for dependencies that are not needed by PETSc. In theory the xxx.py for these packages are marked with

self.useddirectly     = 0    # 1 indicates used by PETSc directly, 0 indicates used by a package used by PETSc

but I see that is missing from boost.py (and likely others). I will fix those. You don't need a --download-boost=no because PETSc doesn't try to install it by default. Packages that are looked for by default are labeled with self.lookforbydefault = 1; they include X (doesn't try to download), 2chtml, hwlock, pthread (doesn't try to download), and ssl (doesn't try to download).

@tgamblin
Copy link
Copy Markdown
Member Author

@BarrySmith: So it sounds like we'll still need to keep the boost arguments for old versions of PETSc where the error persists, or the build will complain, right?

@citibeth
Copy link
Copy Markdown
Member

You don't need a --download-boost=no because PETSc doesn't try to install
it by default. Packages that are looked for by default are labeled with
self.lookforbydefault = 1; they include X (doesn't try to download),
2chtml, hwlock, pthread (doesn't try to download), and ssl (doesn't try to
download).

Packages that try to "help" the user by downloading their own dependencies
drive me nuts, in the Spack context. I long for some future day in which
everyone uses Spack and upstream authors learn they don't have to build
their own mini-dependency manager.

Back to the real world... as I said before, packages that download their
own stuff are problematic in Spack for many reasons:

a) It creates two different versions of a package, breaking Spack's
guarantee to provide consistency in its concretization algo.

b) It doesn't work running Spack on off-line supercomputers.

I would set --download-XXX=no for everything you can, just to be safe. No
reason for the Spack package NOT to do that.

Of the things that are looked for by default... are they required? If they
are not found, does the installer (a) go on without them or (b) throw an
error and stop? In general, (b) is a better design, but many packages do
(a) as well.

If the answer is (a), then I would make those things be optional
dependencies, turned on/off by Spack variants. If it is (b), then I would
make them be required dependencies. You might also consider not going to
the effort for some of them, if the dependency isn't yet in Spack and it's
not really core to the package (I'm thinking X here; why does PETSc need X?)

@tgamblin
Copy link
Copy Markdown
Member Author

I would set --download-XXX=no for everything you can, just to be safe. No
reason for the Spack package NOT to do that.

I don't see a reason not to do this either -- @BarrySmith: is there some way to just disable the external downloads entirely?

@BarrySmith
Copy link
Copy Markdown
Contributor

These are the only ones you need turn off --download-c2html=0 --download-hwloc=0

and if you don't want to link against ssl have --with-ssl=0

You don't need to turn off all the --download because the above 2 are the only ones on by default.

@BarrySmith
Copy link
Copy Markdown
Contributor

I don't think you should be in the business of installing PETSc before the 3.6 series. This will simplify somethings. It seems bad to have the petsc.py file be complicated just to handle ancient releases.

@citibeth
Copy link
Copy Markdown
Member

Ancient releases of PETSc are required for ancient versions of application
software that need those releases. Please don't disable them.

What does linking with SSL gain you? Can it be a Spack variant? (Although
admittedly, every system has an SSL so if it's a variant it should probably
be default=True)

On Wed, Oct 12, 2016 at 1:48 PM, Barry Smith [email protected]
wrote:

I don't think you should be in the business of installing PETSc before the
3.6 series. This will simplify somethings. It seems bad to have the
petsc.py file be complicated just to handle ancient releases.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1999 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1cd6vdw4EnrQOmJkCkrDkejTYoSfZLks5qzR1dgaJpZM4KUDQP
.

@tgamblin
Copy link
Copy Markdown
Member Author

@BarrySmith: Thanks. As we get Spack going, the intent is to have it provide some measure of reproducibility, so we'll probably want to do things like this one day. Not oppose to cutting off PETSc before 3.6 right now though. Not sure what @davydden and some of the other dependent library maintainers need.

@tgamblin
Copy link
Copy Markdown
Member Author

@citibeth: We currently don't actually have any mainline packages with an upper bound lower than 3.6:

$ egrep 'depends_on..petsc' */package.py
dealii/package.py:    depends_on("petsc+mpi",        when='@8.4.2:+petsc+mpi')
dealii/package.py:    depends_on("petsc@:3.6.4+mpi", when='@:8.4.1+petsc+mpi')
fenics/package.py:    depends_on('[email protected]:', when='+petsc')
gmsh/package.py:    depends_on('petsc+mpi', when='+petsc+mpi')
gmsh/package.py:    depends_on('petsc', when='+petsc~mpi')
slepc/package.py:    depends_on('[email protected]:', when='@3.7.1:')
slepc/package.py:    depends_on('[email protected]:3.6.4', when='@3.6.2:3.6.3')

Do you know of any packages that require 3.5 or earlier?

@davydden
Copy link
Copy Markdown
Member

I am fine with 3.6 and above. On another hand the only conditionals on version in PETSc's package.py are

depends_on('superlu-dist@:4.3', when='@:3.6.4+superlu-dist+mpi')
depends_on('[email protected]:', when='@3.7:+superlu-dist+mpi')

So at this point I don't think it will simplify much.

@citibeth
Copy link
Copy Markdown
Member

@tgamblin see comments at the end of PISM:

https://github.com/citibeth/spack/blob/efischer/develop/var/spack/repos/builtin/packages/pism/package.py

I have not yet coded that information into Spack but it's the reality
nontheless. PISM radically changed its internal structure with version
0.7, and I still have code that needs to run version 0.6 and before.
Things move REALLY slowly in this space because they are so
application-oriented and there are so few users (and programmers).

If supporting old versions is REALLY too hard, one option would be to
rename the old package and start afresh. For example, petsc becomse
petsc-v1 or something. I would have no problem depending on petsc-v1
instead of pestc for older versions of pism.

On Wed, Oct 12, 2016 at 1:54 PM, Todd Gamblin [email protected]
wrote:

@citibeth https://github.com/citibeth: We currently don't actually have
any mainline packages with an upper bound lower than 3.6:

$ egrep 'depends_on..petsc' */package.py
dealii/package.py: depends_on("petsc+mpi", when='@8.4.2:+petsc+mpi')
dealii/package.py: depends_on("petsc@:3.6.4+mpi", when='@:8.4.1+petsc+mpi')
fenics/package.py: depends_on('[email protected]:', when='+petsc')
gmsh/package.py: depends_on('petsc+mpi', when='+petsc+mpi')
gmsh/package.py: depends_on('petsc', when='+petsc~mpi')
slepc/package.py: depends_on('[email protected]:', when='@3.7.1:')
slepc/package.py: depends_on('[email protected]:3.6.4', when='@3.6.2:3.6.3')

Do you know of any packages that require 3.5 or earlier?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1999 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1cd2Sb3rQ7ub0_FBFQb3LiEuU7p7l-ks5qzR7ogaJpZM4KUDQP
.

@BarrySmith
Copy link
Copy Markdown
Contributor

This issue interface would be much better if I could respond directly to specific comments above instead of having to add a new comment at the end and try to connect the context with a comment up 5 comments ago. Is there a way I can attach comments directly to particular other comments?

I reason I suggested cutting off at 3.6 was this seeming dependence on boost which should only exist for older versions of PETSc. This suggestion was motivated by Todd's comment: "@BarrySmith: So it sounds like we'll still need to keep the boost arguments for old versions of PETSc where the error persists, or the build will complain, right?" If you do keep old versions then could you just list the boost dependencies for PETSc versions that need it which would not be 3.6 and 3.7

--with-ssl doesn't buy you anything if great value so I think you should stick to just turning it off in spack

In response to "The reason for (3) was to have quich tests of external solvers/preconditioners. This helped in the past to pick up broken MUMPS build on macOS. If PETS has somewhere tests with, say, laplace equation with superlu-dist, mumps, hypre -- we can switch to running those as native make targets. Even more convenient would be to have those in default "make tests"." This is a good idea, I'll add to our smoke test, tests for the included dependencies such as hypre, superlu_dist, mumps so you will only need to run our smoke test and you'll be assured we are checking the external packages as well..

@davydden
Copy link
Copy Markdown
Member

Is there a way I can attach comments directly to particular other comments?

Not really :-( The best one could probably do is to use quotes (>QUOTED TEXT) and tag people @NAME.

This is a good idea, I'll add to our smoke test, tests for the included dependencies such as hypre, superlu_dist, mumps so you will only need to run our smoke test and you'll be assured we are checking the external packages as well..

Thanks @BarrySmith . Once it is there in the next release, i think we could cut those tests off of Spack.

@tgamblin
Copy link
Copy Markdown
Member Author

@citibeth: thanks -- I don't think it's that hard to keep 3.5 in there.

@BarrySmith:

If you do keep old versions then could you just list the boost dependencies for PETSc versions that need it which would not be 3.6 and 3.7

That sounds good to me. We can put when=:3.5 clauses on the boost dependencies.

@tgamblin
Copy link
Copy Markdown
Member Author

@BarrySmith:

Is there a way I can attach comments directly to particular other comments?

If you comment directly in the code (which was actually why I created the PR) you can have separate threads for each in-code comment. But that's about it.

@davydden davydden mentioned this pull request Oct 17, 2016
@tgamblin tgamblin closed this Oct 19, 2016
@tgamblin tgamblin deleted the xsdk/reviews/petsc branch December 31, 2016 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants