includes: name mangle ramble and include from spack normally #469
Conversation
becker33
force-pushed
the
becker/import-ramble-and-spack
branch
from
45485a0 to
2b25c2c
Compare
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
Overall this looks reasonable but
- There's a couple spots that could use documentation
- The mangling of Ramble-as-a-lib vs. Ramble-as-a-driver is handled implicitly
- I'd prefer some tweaks to the regexes (or at least some explanation in a docstring)
| # If ramble is present but not yet name mangled, do that | ||
| ramble_spack_path = ramble_lib_path / "ramble_spack" | ||
| if not ramble_spack_path.exists(): | ||
| self._mangle_ramble() |
There was a problem hiding this comment.
I think ideally we would conceptually distinguish the case where we clone ramble as a library vs. when we clone it to run it.
- e.g.
_install_ramblecan be called frombootstrap()or.ramble()(viaramble_first_time_setup) - I'd recommend making the "library-ness" a property of the
RuntimeResourcesobject, and move this consideration to_install_ramble()- And maybe rename
_install_rambleto reflect that it can update existing installations (i.e. so that this works for users that do agit pullof Benchpark with an already-established Ramble lib prefix
- And maybe rename
| if not self.spack_location.exists(): | ||
| self._install_spack() | ||
| def _mangle_ramble(self): | ||
| """Name mangle ramble spack.* symbols to allow separate spack imports.""" |
There was a problem hiding this comment.
Can you add a bit of detail: mention we are replacing all instances of spack with ramble_spack, and all instances of llnl with [...].
Might be worth broadly outlining why this is expected to work: I don't think this name mangling would make all parts of Spack work, but I believe they make all the parts of Spack that Ramble uses work.
| files = [ | ||
| f | ||
| for f in fs.find(self.ramble_location, "*") | ||
| if os.stat(f).st_mode & stat.S_IWUSR and not os.path.isdir(f) |
There was a problem hiding this comment.
If we don't own these files, we should probably fail, vs. silently doing nothing.
There was a problem hiding this comment.
I'll add a comment -- the main purpose here isn't to catch files we don't own, it's to catch files that aren't writable (like some things within the .git repo).
There was a problem hiding this comment.
I assume nothing under .git should be modified: I recommend skipping it entirely (and explicitly).
| if os.stat(f).st_mode & stat.S_IWUSR and not os.path.isdir(f) | ||
| ] | ||
| file_filter = fs.FileFilter(*files) | ||
| # Don't replace if it's already replaced or if it's a field in an existing module |
There was a problem hiding this comment.
I think trying to make these regexes idempotent will make them more confusing than they need to be: the existence of the ramble_spack directory can function as a sentinel (if you are worried about CTRL-C, I'd recommend a marker file); if we depend on that, I think we can stop considering _, which will simplify the regex.
Either way, I think this will work as long as:
# spack always does this
import spack.util.spack_yaml
# and never does this (which is allowed)
from spack.util import spack_yaml
But IMO it would be more straightforward to look for [\s]spack (i.e. spaces followed by spack)
I think including some examples of things you intend not to match would be useful (e.g. ...spack_yaml
Description
This PR allows us to import from Spack in the normal way instead of our crazy workaround.
The key is that we name mangle all of the ramble imports from
spack.*toramble_spack.*andllnl.*toramble_llnl.*.Dependencies: FIXME:Add a list of any dependencies.
Fixes issue(s): FIXME:Add list of relevant issues.
Type of Change
Checklist:
If adding/modifying a system:
system.pyfile.github/workflowsIf adding/modifying a benchpark:
application.pyand (maybe)package.pyunder a new directoryfor this benchmark
section
If adding/modifying a experiment:
experiment.pyunder existing directory for specific benchmarkIf adding/modifying core functionality:
.github/workflowsand.gitlab/ciunit tests (if needed)