Open joshuahansel opened 11 months ago
@MengnanLi91 Can you try this example with your parser to see that it resolves the error?
@MengnanLi91 Can you try this example with your parser to see that it resolves the error?
Sure,I'll try and let you know
Still have the error, but now only get one error message instead of two. I think fparse not working properly here
*** ERROR ***
fileB.i:7.5: invalid float syntax for parameter: Postprocessors/test_pp/value=${fparse foo + bar}
@dschwen Any idea what's going on here?
@MengnanLi91 are you going to try and include a fix to this with your parser PR, or will it be a subsequent change? The workaround for this bug is a little ugly - I need to manually do all of my fparses in the secondary input file(s).
@MengnanLi91 are you going to try and include a fix to this with your parser PR, or will it be a subsequent change? The workaround for this bug is a little ugly - I need to manually do all of my fparses in the secondary input file(s).
I'll look into this. My current PR only includes the parameter duplication check. This issue may need more than that.
I found a slight change to fileB.i as below works out fine with no error, but value = ${val}
gives the error. For some reason, when two block is merged, the parser only does one variable replacement instead of two, so we have ${fparse foo + bar}
instead of the real value. The piece I don't understand is why this only happens when there are blocks to merge(If [Postprocessors]
is removed from fileA.i, value = ${val}
works fine too). My guess it may relate to how the hit tree structure looks like when block merges
[Postprocessors]
[test_pp]
type = ConstantPostprocessor
value = ${fparse foo + bar}
[]
[]
@joshuahansel -
I wonder if this is related to this discussion we had on Slack in October about a similar brace substitution situation.
I have copied and pasted the conclusion I came to then regarding that case here:
Looking around a bit, I found this comment on the original brace-expression implementation PR that says:
Brace expressions are evaluated in input-file order (i.e. lexical order) from top to bottom of the input file.
which seems to mean for any substitution-B that depends on another substitution-A, B must be evaluated later in the file than A.
So an example like this will work fine because there's only a single substitution happening:
[Functions] [test_fn] type = ConstantFunction value = ${value_to_use_directly} [] [] value_to_use_directly = 6.0
And something like this will also work fine since the earlier substitution feeds the later one:
value_to_use_directly = ${value_to_use_indirectly} [Functions] [test_fn] type = ConstantFunction value = ${value_to_use_directly} [] [] value_to_use_indirectly = 6.0
However, something like this will not work as the later substitution will not be evaluated until after it's used above.
value_to_use_indirectly = 6.0 [Functions] [test_fn] type = ConstantFunction value = ${value_to_use_directly} [] [] value_to_use_directly = ${value_to_use_indirectly}
Since this only happens if fileA.i
has [Postprocessors]
, after that block is merged I wonder if the input becomes:
foo = 2
[Mesh]
type = GeneratedMesh
dim = 1
[]
[Postprocessors]
[test_pp]
type = ConstantPostprocessor
value = ${val}
[]
[]
[Problem]
solve = false
[]
[Executioner]
type = Steady
[]
bar = 3
val = ${fparse foo + bar}
where combining of the [Postprocessors]
blocks merges the bottom one into the top one moving it above this:
bar = 3
val = ${fparse foo + bar}
If this is the case, then that would effectively put the input in the third scenario from our conversation above.
And the earlier value = ${val}
will not work here as it relies on val = ${fparse foo + bar}
which now comes later.
In other words, the later val = ${fparse foo + bar}
would not be evaluated until after value = ${val}
uses it earlier.
I just ran across this issue, so this is all guess-work based on our earlier conversation without looking back at the code.
But the braces "evaluated in input-file order from top to bottom" combined with the block merge may give some insight.
EDIT:
It seems running moose/framework/contrib/hit/hit merge -output - fileA.i fileB.i
confirms this merge behavior.
It shows Postprocessors/test_pp/value = ${val}
is moved above val = ${fparse foo + bar}
in the merged input.
@brandonlangley Thanks for looking into this! It's making some sense at least. I still don't understand why it gives the error
fileB.i:7.5: invalid float syntax for parameter: Postprocessors/test_pp/value=${fparse foo + bar}
instead of complaining that val
just doesn't exist (yet). So what's the order of operations here? From what I see in Parser.C
, it looks like:
${val}
${fparse blah}
It seems like 1 and 2 are switched, but 3 just doesn't happen for some reason. I'm very confused.
I think the right solution is:
${val}
${fparse blah}
I am not sure if we can do combine inputs without merging. The combine and merge are done together in hit::merge() now. When we do combine inputs without merging, we may end up having something like below: Should we allow this or when should we error out? This means fileA.i val will have different values when running individually and together with B. Depend on where fileB.i val is used, fileB.i val may changed after combining as well
fileA.i
foo = 2
val = ${fparse foo + 3}
fileB.i
foo = 1
bar = 3
val = ${fparse foo + bar}
Combined
foo = 2
val = ${fparse foo + 3}
foo = 1
bar = 3
val = ${fparse foo + bar}
Good point, app-opt -i fileA.i fileB.i
allows for overriding, so we should preserve that. So this mythical "combination" of mine would actually need to merge/override the (nodes?) that already exist and then put the remainder at the bottom of fileA.i
.
As far as being able to do this "combine" thing, how is it done with !include
? I see this as a "combination" but at a point in the file that we choose, and for !include
we are not allowing duplicates.
@joshuahansel -
- Combine inputs without merging
- Substitute
${val}
- Evaluate commands like
${fparse blah}
- Merge within the combined input
Just wanted to note to simplify things for the sake of the conversation, fparse
can be removed from the equation.
I threw together a little test utility that simply
Parser.C
currently doesI tested with input files that only use regular brace expression substitution without using fparse
.
This input file:
foo = 2
[Mesh]
type = GeneratedMesh
dim = 1
[]
val = ${foo}
[Postprocessors]
[test_pp]
type = ConstantPostprocessor
value = ${val}
[]
[]
[Problem]
solve = false
[]
[Executioner]
type = Steady
[]
renders as this after the current Parser.C
flow of brace expression evaluations:
foo = 2
[Mesh]
type = GeneratedMesh
dim = 1
[]
val = 2
[Postprocessors]
[test_pp]
type = ConstantPostprocessor
value = 2
[]
[]
[Problem]
solve = false
[]
[Executioner]
type = Steady
[]
You see Postprocessors/test_pp/value = 2
works since the earlier val = ${foo}
feeds the later value = ${val}
.
However, this input file:
foo = 2
[Mesh]
type = GeneratedMesh
dim = 1
[]
[Postprocessors]
[test_pp]
type = ConstantPostprocessor
value = ${val}
[]
[]
val = ${foo}
[Problem]
solve = false
[]
[Executioner]
type = Steady
[]
renders as this after the current Parser.C
flow of brace expression evaluations:
foo = 2
[Mesh]
type = GeneratedMesh
dim = 1
[]
[Postprocessors]
[test_pp]
type = ConstantPostprocessor
value = ${foo}
[]
[]
val = 2
[Problem]
solve = false
[]
[Executioner]
type = Steady
[]
And Postprocessors/test_pp/value = ${foo}
doesn't work as the earlier value = ${val}
needs the later val = ${foo}
.
So there is nothing special about fparse
and this will happen for any brace expressions after that upward merge.
If it merged the earlier block into the later block instead of the later block into the earlier block, this case should work.
But there are probably other override reasons that blocks need to be merged in that direction that I'm not familiar with.
I didn't write the original block merge / override logic so someone like @dschwen would maybe need to chime in.
Ok, we need to run the substitutions on each input before merging into the preceeding inputs. And that requires to hold onto the map of global variables from prior inputs. I'll check if that can be done. Basically we'd run a substitution in the second input with the knowledge of val
from the substitution run on teb first input.
Reminder for @dschwen checking if this can be done:
And that requires to hold onto the map of global variables from prior inputs.
Would anyone be able to attempt the fix to this?
@dschwen You said
we need to run the substitutions on each input before merging into the preceding inputs.
but wouldn't this have this problem?: if I have foo = 2
and a substitution ${foo}
in fileA.i
and then override foo = 3
in fileB.i
, then I'd end up with the value 2, not the value 3.
Would it be possible to change the merge operation to preserve the order of the nodes in the root being merged in, so that we'd end up with
foo = 2
[Mesh]
type = GeneratedMesh
dim = 1
[]
bar = 3
val = ${fparse foo + bar}
[Postprocessors]
[test_pp]
type = ConstantPostprocessor
value = ${val}
[]
[]
[Problem]
solve = false
[]
[Executioner]
type = Steady
[]
@joshuahansel -
change the merge operation to preserve the order of the nodes in the root being merged in
With your original example app-opt -i fileA.i fileB.i
, I assume the "root being merged in" is fileB.i
in this case.
If the merge is changed to preserve the order of nodes from the latter file being merge in, what should happen if you run:
app-opt -i fileB.i fileA.i
instead of:
app-opt -i fileA.i fileB.i
with the same contents of your original fileA.i
and fileB.i
inputs?
When not overriding parameter values, is the expectation that these should work the same regardless of the file ordering?
app-opt -i fileA.i fileB.i
vs app-opt -i fileB.i fileA.i
My expectation is that it would not work the same because if you had foo = 3
in the first and then used ${foo}
in the second, it may not work if you did fileB.i fileA.i
. However, I don't know if we care about this property or not.
Maybe it would be better to just treat the global variables specially during the merge: just always put them at the top?
My vote is that we do not need to worry about having the property that when not overriding values, app-opt -i fileA.i fileB.i
matches app-opt -i fileB.i fileA.i
, since we don't expect them to match when we are overriding values either.
So my current suggestion is still: change the merge operation to preserve the order of the nodes of the root being merged in, as in the example above.
Any thoughts/objections to this plan?
@joshuahansel -
I took a stab at this and changed the merge logic so any block that exists in both inputs is moved to the end of its siblings.
So with your original input setup:
fileA.i
:
foo = 2
[Mesh]
type = GeneratedMesh
dim = 1
[]
[Postprocessors]
[]
[Problem]
solve = false
[]
[Executioner]
type = Steady
[]
fileB.i
:
bar = 3
val = ${fparse foo + bar}
[Postprocessors]
[test_pp]
type = ConstantPostprocessor
value = ${val}
[]
[]
the result after merging is now:
foo = 2
[Mesh]
type = GeneratedMesh
dim = 1
[]
[Problem]
solve = false
[]
[Executioner]
type = Steady
[]
bar = 3
val = '${fparse foo + bar}'
[Postprocessors]
[test_pp]
type = ConstantPostprocessor
value = ${val}
[]
[]
where the [Postprocessors]
block is placed at its latter location and not the first fileA.i
location like it was previously.
I also tested moose_test-opt -i fileA.i fileB.i
after this merge update and it runs successfully instead of failing with:
fileB.i:7.5: invalid float syntax for parameter: Postprocessors/test_pp/value=${fparse foo + bar}
If you don't see an issue with this strategy, I'll go ahead and put it on a pull request and ask you and @dschwen to review.
Great! The logic seems sound to me. Thanks once again! I'd be happy to review that PR, and I'll let @dschwen decide if he wants to additionally review
@joshuahansel -
After running all of the framework tests locally, I just saw I have nine tests failing after this update that passed previously.
I believe six of these are expected errors being different due to input order changing by command line argument merges.
But three of the failures are these EXODIFF tests that I haven't looked into, I wonder if input order could be affecting this.
transfers/general_field/nearest_node/regular.2d_overlay/projection_needed_receiving ......... FAILED (EXODIFF)
transfers/general_field/user_object/subdomain.2d_overlay/projection_needed_receiving ........ FAILED (EXODIFF)
transfers/general_field/shape_evaluation/subdomain.2d_overlay/projection_needed_receiving ... FAILED (EXODIFF)
I'm going to run the set of modules tests locally to see what the damage is there.
I looked at projection_needed_receiving
. No includes, just CLI args. I tried manually doing the process you described to the test, and it works for me even with the new order. So I'd be curious to compare what those input files look like after the merges to compare my manual ordered input files. Is that something we can do - print the input file after all merges? Note in this case there's a multiapp, so there's a main input file and a sub input file.
@joshuahansel -
For the regular.2d_overlay/projection_needed_receiving
failure, I printed the input after both the old and new merges.
For reference, here is the full command running that test showing all the command line parameters that are being merged:
/path/to/moose/test/moose_test-opt -i main.i
Outputs/file_base=projection_receive
Mesh/second_order=true
GlobalParams/bbox_factor=1.5
AuxVariables/from_sub/order=SECOND
AuxVariables/from_sub_elem/order=SECOND
MultiApps/sub/cli_args='Mesh/second_order=true;AuxVariables/from_main/order=SECOND;AuxVariables/from_main_elem/order=FIRST'
--error-override --no-gdb-backtrace
Some top-level blocks are reordered in main.i
and sub.i
with the new merge strategy from the block moves to the end.
But perhaps more suspicious, the AuxVariables
of both main.i
and sub.i
are in a different order using the new merge.
I know nothing about how this is supposed to work regarding where input order might matter or where it should not matter.
But here is the ordering of the AuxVariables
in both main.i
and sub.i
with the normal / old merge strategy that passes.
main.i
:
[AuxVariables]
[from_sub]
initial_condition = -1
order = SECOND
[]
[from_sub_elem]
order = SECOND
family = MONOMIAL
initial_condition = -1
[]
[to_sub]
[InitialCondition]
type = FunctionIC
function = '1 + 2*x*x + 3*y*y*y'
[]
[]
[to_sub_elem]
order = CONSTANT
family = MONOMIAL
[InitialCondition]
type = FunctionIC
function = '2 + 2*x*x + 3*y*y*y'
[]
[]
[]
sub.i
:
[AuxVariables]
[from_main]
initial_condition = -1
order = SECOND
[]
[from_main_elem]
order = FIRST
family = MONOMIAL
initial_condition = -1
[]
[to_main]
[InitialCondition]
type = FunctionIC
function = '3 + 2*x*x + 3*y*y*y'
[]
[]
[to_main_elem]
order = CONSTANT
family = MONOMIAL
[InitialCondition]
type = FunctionIC
function = '4 + 2*x*x + 3*y*y*y'
[]
[]
[]
And here is the AuxVariables
ordering of main.i
and sub.i
with this new "move to the end" merge strategy which fails.
main.i
:
[AuxVariables]
[to_sub]
[InitialCondition]
type = FunctionIC
function = '1 + 2*x*x + 3*y*y*y'
[]
[]
[to_sub_elem]
order = CONSTANT
family = MONOMIAL
[InitialCondition]
type = FunctionIC
function = '2 + 2*x*x + 3*y*y*y'
[]
[]
[from_sub]
initial_condition = -1
order = SECOND
[]
[from_sub_elem]
order = SECOND
family = MONOMIAL
initial_condition = -1
[]
[]
sub.i
:
[AuxVariables]
[to_main]
[InitialCondition]
type = FunctionIC
function = '3 + 2*x*x + 3*y*y*y'
[]
[]
[to_main_elem]
order = CONSTANT
family = MONOMIAL
[InitialCondition]
type = FunctionIC
function = '4 + 2*x*x + 3*y*y*y'
[]
[]
[from_main]
initial_condition = -1
order = SECOND
[]
[from_main_elem]
order = FIRST
family = MONOMIAL
initial_condition = -1
[]
[]
Notice the AuxVariables
for from_*
were moved after the to_*
since these command line parameters merged them in.
AuxVariables/from_sub/order=SECOND
AuxVariables/from_sub_elem/order=SECOND
MultiApps/sub/cli_args='AuxVariables/from_main/order=SECOND;AuxVariables/from_main_elem/order=FIRST'
Both inputs have more reordering, but I figured I'd first ask about AuxVariables
first and have no idea if that might matter.
If block or sub-block order can matter sometimes, then I wonder if a better strategy may be this other idea you mentioned:
Maybe it would be better to just treat the global variables specially during the merge: just always put them at the top?
@joshuahansel -
After a lot of time spent testing many scenarios, I have concluded input order does affect the outcome in various cases.
I verified my assumption above about reordering of AuxVariables
in main.i
and sub.i
causing that EXODIFF failure.
You can quickly see this one example of input order mattering simply by:
[to_sub]
and [to_sub_elem]
to the top of the [AuxVariables]
block in test/tests/transfers/general_field/nearest_node/regular/main.i
[to_main]
and [to_main_elem]
to the top of the [AuxVariables]
block in test/tests/transfers/general_field/nearest_node/regular/sub.i
./run_tests --re=nearest_node/regular.2d_overlay/projection_needed_receiving
AuxVariables
makes the projection_receive_sub1_out.e
EXODIFF failFor projection_receive.e
, projection_receive_sub0_out.e
, projection_receive_sub2_out.e
, the EXODIFF passes.
I haven't looked further into if the failure is caused by the reordering in only one or both of the main.i
and sub.i
files.
You can see a second quick example of an input order dependency that this merge strategy revealed by:
[reporter_forward]
to above [file]
in the [Positions]
block in test/tests/positions/creating_multiapps/apps_from_positions.i
./run_tests --re=positions/creating_multiapps.initial_positions
Positions
makes the (previously passing) run fail with this error:
Unable to locate Reporter context with name: file/positions_1d
Those are only two examples of order dependencies that the framework and modules tests revealed with block moving.
There are many failures I have not looked into, but I think it is safe to say that reordering blocks in the merge is not wise.
As I mentioned, I might take a look at this other idea you had if I have time to see if it could perhaps be a better strategy.
Maybe it would be better to just treat the global variables specially during the merge: just always put them at the top?
Another thought I had was trying to just move the top-level blocks when merging instead of moving sub-blocks around.
But I'm not sure how much time I can spend on it right now unless it's a high priority, @dschwen may have an idea too?
Thanks for investigating this. I feared that input order might be having an impact. I wasn't aware of the ones you've encountered, but I've seen something like this happen before. This sounds like possible bug(s) that the MOOSE team needs to fix. My suggestion would be to wait for us to try and address the order dependency bugs before you invest any more time. When we get that fixed, or if we find that order dependency is by design for some reason (I can't think of why it would be), then we can think on other merge solutions.
Tried your first example. All you need to see an exodiff failure is move [to_main]
in the sub input file.
@GiudGiud I'm looking at your nearest_node/regular.2d_overlay/projection_needed_receiving
test (see above). It seems there is a warning about equidistant points. The test has allow_warnings = True
. Do you see any possibility that with different aux variable ordering in the sub, different values could be chosen?
If there are equidistant points, the result of the transfer is indeed ill-defined. So anything goes, a round-off error in one or the other direction can change the output of the test. But we have been consistently rounding the same way before so the test has not needed maintenance.
AuxVariable ordering though, I don't know how much that would matter
Maybe AuxVariable ordering could cause a change in round-off? But
transfers/general_field/user_object/subdomain.2d_overlay/projection_needed_receiving
does not have an equidistant warning, so I guess it's not necessarily related to that. It's interesting that the 3 diffs are projection_needed_receiving
tests.
It is for sure. We involve the generic projector on the receiving side
@brandonlangley You said
I believe six of these are expected errors being different due to input order changing by command line argument merges.
I'm not sure exactly what you mean. Are they easy fixes? Are these 3 the only obstacles now?:
transfers/general_field/nearest_node/regular.2d_overlay/projection_needed_receiving ......... FAILED (EXODIFF)
transfers/general_field/user_object/subdomain.2d_overlay/projection_needed_receiving ........ FAILED (EXODIFF)
transfers/general_field/shape_evaluation/subdomain.2d_overlay/projection_needed_receiving ... FAILED (EXODIFF)
@GiudGiud If the projection_needed_receiving
tests are confirmed to be faulty, I'm inclined to make an issue and skip these tests so that we can move forward. Would you object to that?
I dont think we can skip tests like this.
I think we can rework the test to be in the configuration with a stable ordering of the variable, and get the tests to pass and create an issue for the problem at hand
I don't understand what you mean by "stable ordering" - do you mean stable as in we switch the order in those inputs right now so that the order doesn't change with Brandon's changes?
As I understand it (see slack), the test has an incorrect gold, even with its current ordering, which is why I suggested the temporary skip, since we're currently just enforcing a specifically wrong behavior 😄 My opinion is that if the test isn't some obscure corner case, and others would actually be impacted by it, then it should be fixed soon, rather than us skipping it indefinitely. If it is an obscure corner case, then I think it's more acceptable to skip and conservatively put an error message in the code if the corner case is encountered. If this is a priority fix, will you be able to look into it soon? I'm guessing this is your area. I can attempt too if you don't think the learning curve is too steep.
@joshuahansel -
@brandonlangley You said
I believe six of these are expected errors being different due to input order changing by command line argument merges.
I'm not sure exactly what you mean. Are they easy fixes? Are these 3 the only obstacles now?:
transfers/general_field/nearest_node/regular.2d_overlay/projection_needed_receiving ......... FAILED (EXODIFF) transfers/general_field/user_object/subdomain.2d_overlay/projection_needed_receiving ........ FAILED (EXODIFF) transfers/general_field/shape_evaluation/subdomain.2d_overlay/projection_needed_receiving ... FAILED (EXODIFF)
I was talking about some of the RunException
tests that are checking for expect_err
expected failure errors.
The test harness uses this basic setup for almost all testing to merge in command like argument parameters:
moose_test-opt -i main.i block_m/param_m=value_m block_n/param_n=value_n
And with this update, that effectively moves [block_m]
and [block_n]
to be processed after all other blocks.
Since this changes the order the input is processed, some error check tests now fail due to a different condition.
A [block_m]
error condition test may now fails because of something else before [block_m]
is processed.
This would required re-baselining the expect_err
field in these RunException
tests to now be the earlier error.
Note that I did not go through all of the framework test failures one-by-one to verify the cause of each one.
And there were also a slew of modules tests that were failing which I did not go through to see the reasons why.
And, of course, there are likely many other tests I haven't run in downstream apps that this update would break.
So I'm not sure that moving the input blocks to the end of its siblings is the best solution to fix this issue.
It seems a bit like a hack to band-aid the issue when updating the merge + substitution logic would be better.
Also projection_needed_receiving
are not the only issues, as mentioned above here is JSONDiff
test failure.
You can see a second quick example of an input order dependency that this merge strategy revealed by:
- moving
[reporter_forward]
to above[file]
in the[Positions]
block intest/tests/positions/creating_multiapps/apps_from_positions.i
- running
./run_tests --re=positions/creating_multiapps.initial_positions
- seeing this reorder of
Positions
makes the (previously passing) run fail with this error:Unable to locate Reporter context with name: file/positions_1d
Those are only two examples of order dependencies that the framework and modules tests revealed with block moving.
There are many failures I have not looked into, but I think it is safe to say that reordering blocks in the merge is not wise.
I don't understand what you mean by "stable ordering" - do you mean stable as in we switch the order in those inputs right now so that the order doesn't change with Brandon's changes?
Basically. If with Brandon's changes we now have the right result.
It joins a long and growing list of issues and things to do. I m always happy to have someone take something off my list. I certainly wont work on it this week or the next, there's two papers I need to tend to.
@brandonlangley Yeah, you're probably right that it isn't the best course of action. It bothers me philosophically that we have tests that rely on some order (and I'm glad we discovered a bug), but I think you're right that this transition could be a lot of work considering possible downstream failures.
@GiudGiud I figured you were pretty busy - I can try and understand general field transfers to try to reduce your burden. I'll make an issue for now. In any case, it's not urgent to this issue anymore, as we'll seek a different solution that doesn't change order.
Bug Description
When using the multiple input file capability
substitutions do not occur correctly. See example below.
Steps to Reproduce
fileA.i
:fileB.i
:This gives the error:
Impact
There is a workaround: duplicating input files, but this is not a long-term solution.