Conversation
Yurlungur
left a comment
There was a problem hiding this comment.
Thanks for your hard work on this @bprather ! It seems like it was a lot harder to get right than you had hoped.
It also seems like you've combined a LOT of things in this MR. The simple TOML++ replacement has been mixed in with additional features. Perhaps these should be split up? For example, do the TOML overhaul in one MR and then the multiple input files + Providence tracking in a separate MR? They seem separable. That would help me at least read through this MR and provide useful feedback. I think it would also make it easier to discuss all the stuff you want to add here.
Also given you based this on my MR #1266, the diff will probably look cleaner if you merge into that branch, rather than develop.
However, this may break features which relied on ambiguity: for example in Parthenon, one parameter allowed strings "true" "false" and "timestamp" as valid inputs -- as the one who wrote that input I can say with confidence that nonsense was bad and it's good that it's now impossible.
I am fine paying this price. You're right that the original version was not great.
The new system comes with a number of advantages. As noted, "blocks" (tables) can now be nested arbitrarily, and iteration within a table is local. Thus within Parthenon one could call pinput->Blocks("parthenon") to get the tables under keys "job", "output0", "time", and so on and so forth, each of which may define sub-tables similarly iterable.
I can see the advantage to this but I honestly don't like it. It introduces a lot of extra complexity compared to a "flat" two-level structure. That said if that's how TOML works and we want to move to TOML, maybe that's the price we pay.
Parameters now have canonical paths
👍
as well as for arrays which might contain multiple types (no idea why you'd actually do the latter, but it's allowed in the standard for some reason).
I think we should explicitly forbid this. We're not Python.
Additionally, the origin of each parameter is tracked in a table parallel to the parameters themselves, containing only strings. Calling GetOrigin("path") tells you what set a parameter, between { restart, input, cmdline, code, defaultvalue }. GetOriginFile gives you which input file set the parameter, if it was an input file. The loop processing multiple input files is WIP still but everything else is in place to pass as many -i file1.par -i file2.par etc. as you want.
This is cool, but is there a reason to do it with a parallel TOML table? Wouldn't a std::map<std::string, ParameterOrigin> be better? Also I dunno how I feel about allowing multiple input files... I guess I'm worried that this seems like a lot of additional complexity for something that can, fundamentally, be solved with the unix cat utility.
That is, an input file with block/var = x and later block/var = y will error, but if the former is defined in a restart file and overridden with an input, or overridden on the command line, that is expected.
👍
What about two separate input files? That probably should error.
Code compatibility should be drop-in, all of the same function calls work the same as before, except anything relying on type ambiguities as mentioned above. The one exception is when iterating over blocks looking for a pattern. Examples of the replacement pattern for iterating over blocks can be found in the updated mesh.cpp and output_package.cpp.
👍
| endif() | ||
| endif() | ||
| endif() | ||
| include_directories("external/tomlplusplus") |
There was a problem hiding this comment.
Is this the preferred way to do it? Not add_subdirectory? How does this work for installing parthenon as a library?
There was a problem hiding this comment.
It's header only, so I assumed this was how to do it. But there are a ton of installation methods on the website, maybe another works better? Didn't really think about this.
There was a problem hiding this comment.
I would do add_subdirectory and then add TOML as a CMake target, the same way we add Kokkos. That should automatically thread it through all of the nonsense CMake might rely on.
There was a problem hiding this comment.
The website doesn't explicitly call that approach out, but I see the cmake supports it. There's checks about whether or not the project is "top level."
There was a problem hiding this comment.
Ok I tried messing with this a bit and it doesn't seem like TOML++'s build system supports this. So we should leave it for now and I'll submit an issue.
| for (auto pib : pin->Blocks("parthenon")) { | ||
| std::string block_name = std::string(pib.first); |
There was a problem hiding this comment.
Just to confirm, you could have instead done a search for "parthenon/static_refinement" here right? You just chose to use the nested block structure?
There was a problem hiding this comment.
Actually, not really. Asking for Blocks("") would give you just "parthenon" and e.g. "problem" or whatever. If you need a multi-level recursive list you're encouraged into the visitor pattern.
There was a problem hiding this comment.
hmm I don't love that... oh well... price we pay maybe.
There was a problem hiding this comment.
Why is this a price? I thought separating Parthenon blocks was the whole reason for naming them all parthenon/ in the first place -- this just enforces that structure.
I can't think why anyone would name their blocks as if they had a structure, then want to iterate as if that structure weren't there, but if there's a use case it's definitely possible to emulate. Just didn't see it as useful.
There was a problem hiding this comment.
I just want the ability to loop over all blocks, which is something someone might want to do. But if you've added a visitor that visits everything, then it's a non-issue.
| "is undefined behavior. If you don't know how this was triggered, please contact " | ||
| "the Parthenon developers."); | ||
| pinput->SetBoolean("parthenon/job", "run_only_analysis", arg.analysis_flag); | ||
| pinput->ParameterDump(std::cerr); |
There was a problem hiding this comment.
didn't know this was possible, neat. Wonder if we should have a flag for that. Just spit out everything from all sources. If we're going to allow a bazillion of them, it would be nice to have parthenon combine them into a single "master" object we could read telling us exactly how the simulation was set up. (though I guess this gets dumped to restart)
But probably this should be guarded behind a flag, right?
There was a problem hiding this comment.
Happy to write any of that!
| v.erase(std::remove(v.begin(), v.end(), ' '), v.end()); | ||
| v.erase(std::remove(v.begin(), v.end(), '\''), v.end()); | ||
| // std::cerr << "INSERTING: " << path << "=" << v << std::endl; | ||
| if (std::count(v.begin(), v.end(), ',')) { | ||
| // Record an array by adding the necessary TOML | ||
| try { | ||
| v = std::regex_replace(v, std::regex(","), ", "); | ||
| new_tbl = toml::parse(path + " = [" + v + "]"); | ||
| } catch (const toml::parse_error &err) { | ||
| v = std::regex_replace(v, std::regex(", "), "\", \""); | ||
| new_tbl = toml::parse(path + " = [\"" + v + "\"]"); | ||
| } |
There was a problem hiding this comment.
what joys maintaining backwards compatibility is...
| inline void recursive_merge(toml::table &a, const toml::table &b, const toml::key &key, | ||
| T &&el, bool check_dups) { |
There was a problem hiding this comment.
I'm kind of disappointed by how much work you have to do to do this with TOML. It really feels like this should be functionality the library provides. In the end I don't think we're going down LOC in our parser. I was really hoping we would. To me that was part of the value proposition.
There was a problem hiding this comment.
I was disappointed, too. Partially this was a consequence of using nested toml::tables as the data store, and not something with more features. Maybe something like std::map<std::string, std::variant<double, int64_t, etc, etc>> would work, and would implement some of these things for us.
There was a problem hiding this comment.
Yeah.. or std::map<std::string, std::any>?
Well now that you pared this MR down to just the TOML port, it looks like we are way down in LOC so I am back to happy about that. But still, yeah, disappointing TOML doesn't provide a merge table feature.
There was a problem hiding this comment.
very satisfying to see all the red in this file!
There was a problem hiding this comment.
very satisfying to see all the red in this file!
|
I am happy to rip out the origin tracking for now and make this a smaller changeset if that's preferred! It's only this way right now because I was developing the changes and features together a bit, to make sure the library was capable of doing the things I wanted, which it often seemed like it was not. Re: nesting blocks, I think that's the biggest added complexity and source of possible bugs here. I am looking forward to having it available: for example, several of my blocks deal with corrections to a simulation, and each takes a list of "floors" describing when to trigger the fix, written in the same basic form. Having these as sub-blocks, rather than alongside other parameters at the root level, would mean I could finally enforce one base-level block per package and every package reads only its block, which would be nice for separability. I am happy to optionally enforce the old two-level form: it would only require nixing the current replacement '/'->'.', then writing a check of the parsed table. But a check wouldn't solve the need to code for nested tables in Parthenon. The recursive visitor thing going on in Re: multi-type lists, agreed. You already can't work with them (e.g. Re: the origins table, I was planning to write it out to restart files, so I wanted the serialization. But a map might be better, and now I'm not sure this info should be written to restarts... A map to the Re: two input files, you're right but I hate it, because we're going to have to merge all the input files' parameters and error on overrides (by default), before we then merge them to the restart file parameters and don't check for overrides (by default). And of course we can't override those defaults without parsing the parameters first... |
Sounds good... yeah lets split it up into smaller units of work. I think future us will thank us.
If this is something you think you need downstream then so be it... Parthenon exists to support downstream codes. Do other downstream codes want this? @acreyes @adamdempsey90 @pdmullen @jdolence @pgrete ?
Given this is built into TOML I'm not sure us enforcing it buys us anything. It also becomes a non-issue if the a
👍 sounds good
Ok let's table the provenance question for now. I can try to add the capability you wanted, now that I understand it, based on what you have here and what I was doing for docstrings in my MR. I agree provenance + docstring checking is kind of a single unit of functionality under the hood.
I know but I think if we want multiple input files we absolutely have to check for duplicate parameters because that's the most likely source of errors, IMO... duplicate parameters running around in multiple copy-pasted files. |
|
My only comment right now is to please add a cmake compile time option to use the old parser without any modifications. Downstream codes should be able to opt in to this breaking change. |
|
Okay, all "shadow table" functionality related to origins is ripped out, though I left the interface stuff since it reflects what I think are the minimum distinctions to keep. Also tabling multiple input files until origin info is restored -- I absolutely agree checking for duplicate params between input files is necessary, but it sucks to implement and I think it will depend on how the origin information is eventually kept. (Idea would be: when merging both parameter tables and their lists of origins, if two parameters conflict you can check their origins and if both are from files, error). Will focus on the currently failing tests, and adding new checks and tests for our agreed bad (but valid TOML) inputs. |
Is there some specific break I can address? I've really tried to minimize any breakage for downstreams, and I don't anticipate anything major for my case. A flag to use the old parser completely unmodified would mean also storing the parameters the old way (all strings), which requires effectively keeping around the old versions of So if there's any individual discrepancy I can fix, that's likely to be easier for me, benefits everyone, and provides less maintenance burden than two full codepaths. |
Yurlungur
left a comment
There was a problem hiding this comment.
With the origin and multiple input files logic removed, I'm happy to endorse this. I'm still disappointed that TOML makes us do as much manually as it does, but this is still a big improvement as far as amount of code we have to maintain, IMO.
That said, I would like my suggested inline changes addressed. I also understand folks reticence since it is somewhat breaking. So would like to know all the downstreams are happy.
| endif() | ||
| endif() | ||
| endif() | ||
| include_directories("external/tomlplusplus") |
There was a problem hiding this comment.
Ok I tried messing with this a bit and it doesn't seem like TOML++'s build system supports this. So we should leave it for now and I'll submit an issue.
| paths.push_back(toml::path(prefix.append(key.str()))); | ||
| } | ||
| } | ||
| std::vector<std::string> ParameterInput::GetAllPaths(toml::table &a) { |
There was a problem hiding this comment.
should there be a version of this that just returns all paths for the internal table?
There was a problem hiding this comment.
Sure! This function was only written for your use writing origin and access tracking, so whatever calling convention you need is fine
After some time to digest parts of this, it looks like it's backwards compatible with the input file format (which was my main concern), so as long as this is tested thoroughly for bugs, I don't have any major hangups. It will take some effort to get artemis working with this, but I don't think it will be too bad.
that would be useful for my compiler. |
Ah, right. It can, but by default
Absolutely will give it a shot, yeah. The repo mentions it optionally uses some C++20 features if it's enabled, so I'm not worried here.
It does! I'm adding an optional check to forbid them as a part of the table checker, but I plan to allow them in KHARMA too. I'm realizing I'll have to modify the |
…kref-boogaloo Add ParameterRef
…rings-2-blockref-boogaloo Revert "Add ParameterRef"
…pc-lab/parthenon into blb/toml
|
I've rebased this on top of @Yurlungur's #1283, which in the process considerably simplified aspects of that PR, and I think I could cut it down the line count further while maintaining its functionality and supporting parameters outside blocks, new parameter types, etc. The TOML table was designed for the sort of things implemented over there (especially type management), so it integrates nicely. The test failures are almost all due to my nesting calls to I really think this raises the question of whether we should just put any further parameter changes for a while atop this PR, which I swear makes things easier once you get used to it! |
…kDesired test commented
|
I have updated this branch to function on top of the docstrings branch, re-enabled provenance and CheckRequired and CheckDesired, and extended provenance, so that now the source is properly recorded and reported when checking orphans. All tests pass now except the output test, which is expected to break. |
|
After discussion with @bprather I also added the functionality to stash raw inputs into the params of an Detailed provenance information is available, and printed for things like warnings (for CheckOrphans for example) but is not otherwise printed or used. It could be though. |
@adamdempsey90 does the ordering need to persist across restarts? Presumably this would be desirable? |
Probably not. The one place in artemis where this might matter stores things with a map instead of an ordered vector, so it should be fine with it being out of order. That being said, people might expect things to be in the input order always. |
PR Summary
This started at the meeting as a method of addressing a number of different issues related to parameter parsing and storage.
It adds toml++ as a dependency managing parameter parsing and storage (in nested toml::table objects representing arbitrarily-nested versions of the old parameter "blocks"). Thus new Parthenon input files can either be in TOML as well as in the old input format.
Unfortunately, turning the very loose Athena/Parthenon input language into a strict standard meant confronting types. In the old parameters, everything was a string until it was used by the code (meaning, among other hilarities, the same parameter could get read as boolean and string in the same code, and single-element lists had zero distinguishing features from a string).
toml++ stores variables with a limited set of types conforming to the TOML standard, so there is now a concept of "right" and "wrong" type with which to fetch something. Thus, for example,
.true.is no longer a valid boolean evaluating tofalse. However, this may break features which relied on ambiguity: for example in Parthenon, one parameter allowed strings "true" "false" and "timestamp" as valid inputs -- as the one who wrote that input I can say with confidence that nonsense was bad and it's good that it's now impossible. Incidentallyparthenon/job/archive_parameters=timestampis now expressedparthenon/job/archive_parameters=trueandparthenon/job/archive_timestamp=true.All that to say backward compatibility is best-effort, though it works perfectly for the tests.
The new system comes with a number of advantages. As noted, "blocks" (tables) can now be nested arbitrarily, and iteration within a table is local. Thus within Parthenon one could call
pinput->Blocks("parthenon")to get the tables under keys"job","output0","time", and so on and so forth, each of which may define sub-tables similarly iterable. For more complex recursive tasks involving parameters of different types,toml::tabledefines a visitorfor_each, examples of which can be found inparameter_input.hpp.Parameters now have canonical paths
"block1.block2.name"which can always be used to get them. There are now genericGet<T>("block", "name")andGetPath<T>("block1.block2.name")implementations for getting parameters beyond justint,Real, andvector-- these can be used to write and read your own parameters of any type, e.g. your owntoml::tables, as well as for arrays which might contain multiple types (no idea why you'd actually do the latter, but it's allowed in the standard for some reason).Additionally, the origin of each parameter is tracked in a table parallel to the parameters themselves, containing only strings. Calling
GetOrigin("path")tells you what set a parameter, between{ restart, input, cmdline, code, defaultvalue }.GetOriginFilegives you which input file set the parameter, if it was an input file. The loop processing multiple input files is WIP still but everything else is in place to pass as many-i file1.par -i file2.par etc.as you want.Repeats are now checked within a single source (restart, input file) but not between sources. That is, an input file with
block/var = xand laterblock/var = ywill error, but if the former is defined in a restart file and overridden with an input, or overridden on the command line, that is expected. Though, you can optionally warn on all overridden parameters in case this is desirable behavior.I think that's it for new features, except that the file reading is mildly simplified and hashing is simpler than it was, while maintaining (I think) exactly the same properties (not dependent on order parameters were added, but still dependent on all keys and values).
Code compatibility should be drop-in, all of the same function calls work the same as before, except anything relying on type ambiguities as mentioned above. The one exception is when iterating over blocks looking for a pattern. Examples of the replacement pattern for iterating over blocks can be found in the updated
mesh.cppandoutput_package.cpp.This is very much still WIP, comments welcome.
Current TODO list:
GetAllPathscall to retrieve all leaf elements' full paths, for whenever Parthenon needs every parameter with a unique associated keyparameter_lists_store into a feature (i.e. retrieving "how did the restart file set this parameter, even though the new input file overrode it?") or ditch it.toml::parseresults which explicitly forbids dates, mixed arrays, or optionally enforces the old two-layer block/var structure of tables.Blocks()return onlytoml::tableeven if there are other top-level valuesarchive_parametersparameter change.And write new tests of:
PR Checklist