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

Geometry upgrade separate packages #179

Merged
merged 94 commits into from
Feb 7, 2024

Conversation

mrhardman
Copy link
Collaborator

Pull request to merge my own merge of geometry-upgrade and the branch which restructures the packages which make up moment_kinetics.

Made for comparison to try to find errors.

Avoids needing to import unnecessary dependencies (Plots.jl when using
Makie.jl, or Makie.jl when using Plots.jl, or either when only running
simulations), which should reduce precompilation time, and the time to
build system-images.

Also removes a few unused dependencies.
It is inconvenient to provide the option for runtime plots when
splitting the post-processing functionality into separate packages. The
option was not used much anyway. It should be possible to add an
'extension' (with Julia-1.9.x) that provides the option if a
post-processing module is loaded, if we want to add it back in future.
Allows us to separate the Project.toml for the moment_kinetics package
from the Project.toml that a user sets up to run the code.
NetCDF file I/O is now done by an 'extension' that is available only if
NCDatasets is installed.
The 'generic' definitions are intended only to define a name that can be
imported by each implementation to add an implementation-specific
method. If they are defined with brackets after the name (as several
were previously) then there is a zero-argument method of the function
that does nothing, which might sometimes be confusing. Therefore remove
those brackets.
The TimerOutput argument is not needed any more, so do not need the
TimerOutputs dependency.
This function is not available as Plots.jl is not included as a
dependency of moment_kinetics any more.
Also bumps the Julia version used for the CI to 1.9, as this is required
in order to use package extensions.

Also adds NCDatasets to test dependencies. This will require/install
NCDatasets when running `Pkg.test()`, but it should still be possible to
run the tests without NCDatasets by just doing
`include("moment_kinetics/test/runtests.jl")`.
The 'modern' way to specify extra dependencies for the tests in Julia is
using `test` in the `[targets]` section of the top-level Project.toml.
The old method of using an extra `test/Project.toml` is described as
'the old method' which, while it will continue to be supported in
Julia-1.x, seems to be out of favour.

The main point of this update though is to remove extra dependencies
from the tests, which are not needed.
NCDatasets is now an optional dependency (also Symbolics, but that is
not used in the test suite). To allow the tests to run when NCDatasets
is not installed, the tests that use NetCDF now fall back to HDF5 if
NetCDF is not available.

The fall-back can be disabled explicitly by passing the
--force-optional-dependencies command line flag, to ensure that the CI
tests do actually use NetCDF I/O, and will error if it is not available.
macOS parallel tests are very slow (at the moment), so only run on 4
processes (not 3 or 2), and do not run long tests.
Updates the machine setup scripts in the `machines/` subdirectory and
the precompilation scripts to account for the new package structure
(with postprocessing tools moved into separate packages).

Also adds a 'generic-pc' option that can be used to set up
moment_kinetics on a standard Linux desktop/laptop machine.
This is convenient when the user chooses to have their .julia locally in
the project directory.
When not on a cluster with a batch job submission system, the shell
scripts to submit jobs are just clutter in the top level directory. Move
the scripts to machines/shared, and symlink them at the top level when
we set up moment_kinetics for a batch system.
This change puts all the prompts where interactive user input is
required to the same place, near the beginning of the script, so that
the user can go away and do other things while (potentially) slow parts
like precompilation are running.
Allows for setting up separate project directories for post-processing,
which will be useful for example when the different projects need
different optimization flags.
Ensures precompilation is done with the same flags as will be used
later (or at least should be). This is important as precompilation of
dependencies will be (or may be) re-done if the flags change.
This should be simpler than maintaining a separate script for each
machine. Now machine-specific settings are saved in a
machine_settings.toml file for each configuration.
Also simplifies the finding of the julia executable. The positional
argument to machine_setup.sh and the '-d' flag are removed - the script
just prompts the user for input, and remembers the settings so that they
are provided as the default if the machine_setup.sh script is re-run.
johnomotani and others added 13 commits January 7, 2024 11:51
There may be some routines that follow different code paths for the
first or last element, so try to make sure all code is used.
Means collision operator code gets compiled and added to system image
`moment_kinetics.so`.
Needs to be a bash script so we can activate the Python venv within it
when enabling plots_post_processing.
…of one core), but which seems to have a bug in the 1D-mirror MMS test for multiple cores (this could be due to compilation issues).
@mrhardman mrhardman requested a review from johnomotani February 4, 2024 22:57
@mrhardman
Copy link
Collaborator Author

My main observation doing this merge was that catching errors in the merge of the external manufactured solutions module was annoying due to unclear error messages. Perhaps because I am building manually, I had to iterate several times to get both Symbolics and MPI to build consistently.

@johnomotani
Copy link
Collaborator

My main observation doing this merge was that catching errors in the merge of the external manufactured solutions module was annoying due to unclear error messages. Perhaps because I am building manually, I had to iterate several times to get both Symbolics and MPI to build consistently.

That's odd, I haven't had similar problems. Do you remember what it was that didn't work? We could maybe improve the manual setup instructions. To be honest, I hadn't really expected anyone to try to follow them! Hopefully the setup script is a record of one set of operations that does work!

@mrhardman
Copy link
Collaborator Author

At least in the most recent example, when I used git pull to update the local repo I had to reinstall MPI and Symbolics in the package manager before I could run a simulation. In the end everything worked, but I had to know that that was the problem that needed fixing!

@mrhardman
Copy link
Collaborator Author

P.S. In principle, I would want to support the manual setup instructions in addition to your scripts. In the event that your script fails to work it would be much easier for a new user to follow manual setup instructions (and potentially write their own script) than to back out from your script what they need to do.

@johnomotani johnomotani changed the base branch from separate-postprocessing-packages-merge-geometry-upgrade to geometry-upgrade February 6, 2024 13:34
Copy link
Collaborator

@johnomotani johnomotani left a comment

Choose a reason for hiding this comment

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

👍

@mrhardman mrhardman merged commit 4c1415d into geometry-upgrade Feb 7, 2024
15 of 16 checks passed
@johnomotani johnomotani deleted the geometry-upgrade-separate-packages branch February 14, 2024 14:26
@johnomotani
Copy link
Collaborator

At least in the most recent example, when I used git pull to update the local repo I had to reinstall MPI and Symbolics in the package manager before I could run a simulation. In the end everything worked, but I had to know that that was the problem that needed fixing!

Right, I think this is a documentation issue. BTW, if you try to use MMS features without having installed Symbolics, you should have got an error message suggesting that you install Symbolics. If that message didn't appear, that's a bug I should fix...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants