arup-group / mc

Making MATSim Configuration fun again
MIT License
6 stars 1 forks source link

Should the debug subcommand return an error return code when config validation fails? #5

Closed mfitz closed 3 years ago

mfitz commented 3 years ago

When using MC's debug subcommand against a Test Town MATSim config file, I see a validation failure:

$ mc debug /Users/mickyfitz/workspace/matesto/data/matsim12-test-town/test_multimodal_config_simplified_network.xml

---VALIDATION FAILURE---
ACTIVITIES {'dropoff_1', 'dropoff_3', 'dropoff_2', 'depo'} missing in: low income
ACTIVITIES {'dropoff_1', 'dropoff_3', 'dropoff_2', 'depo'} missing in: medium income
ACTIVITIES {'dropoff_1', 'dropoff_3', 'dropoff_2', 'depo'} missing in: high income
ACTIVITIES {'home', 'work'} missing in: freight
----------DONE----------

If I check the process return code, I see the non-error 0 value, meaning the MC process terminated normally:

$ echo $?
0

Given that the config in question does not cause any fatal error when later used by Columbus/MATSim, perhaps this is okay. Maybe the validation failures can be considered warnings, rather than errors. But if the errors in the config file were more serious, should the process exit with an error? Perhaps we can add an optional flag to the debug subcommand, off by default, that tells MC to exit with an error if validation failed.

The reason I care about this: when I used MC in a Matesto pipeline to validate/debug a MATSim config file, the relevant excerpt from the resultant Matesto/Popper log looked like this:

[mc-noop-testtown12-] docker create name=popper_mc-noop-testtown12-_9bb0c56c image=758645626094.dkr.ecr.eu-west-1.amazonaws.com/mc:latest command=['debug', 'matsim12-test-town/test_multimodal_config_simplified_network.xml']
[mc-noop-testtown12-] docker start

---VALIDATION FAILURE---
ACTIVITIES {'depo', 'dropoff_2', 'dropoff_3', 'dropoff_1'} missing in: low income
ACTIVITIES {'depo', 'dropoff_2', 'dropoff_3', 'dropoff_1'} missing in: medium income
ACTIVITIES {'depo', 'dropoff_2', 'dropoff_3', 'dropoff_1'} missing in: high income
ACTIVITIES {'work', 'home'} missing in: freight
----------DONE----------
Step 'mc-noop-testtown12-' ran successfully !

Note that Popper considers the process invoked in the container to have been successful (Step 'mc-noop-testtown12-' ran successfully !) - this is because the process return code was 0. In the context of a Popper workflow, this means that, rather than halting, the workflow continues on to the next step, which in this case was network simplification via GeNet. It would be nice to have some kind of "strict" option where I can make the pipeline halt if config validation fails.

fredshone commented 3 years ago

Thanks @mfitz. I've modified the debug method so that the terminal message is clearly a warning, eg:

-----------WARNING-----------

In the case of debug, I prefer not to raise an error with the current functionality as they are more 'suggestions'. For example in the example you show, the config is fine. In future if we identify some 'deal-breakers' in the bebug method then we could throw an error with these.

Re validation - MC essentially validates all module, paramset and param names at read or modification time. This is primarily to prevent typos but also flags new modules that MC does not expect.

So the best ways to get an error are to 'corrupt' a module name, either (i) in the raw xml or (ii) using MC to make a modification after config is read (examples in the Readme).