-
Notifications
You must be signed in to change notification settings - Fork 8
Settings Checker for Extensions #31
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
base: main
Are you sure you want to change the base?
Conversation
|
@andkay please merge in latest changes from |
|
@jpn-- I've forked and rebased this branch to |
jpn--
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked over the code changes here. Conceptually, it looks good. It passes the tests now, which is good. But to really test it I tried writing another test that would intentionally break an extension settings file:
In short, we make a copy of the settings, break the av_ownership.yaml file, and run with the broken settings. It should raise a validation error before it gets to the av_ownership model. But it doesn't: https://github.com/driftlesslabs/sandag-abm3-example/actions/runs/17132819984/job/48601266109
Can we figure out why, and fix it?
|
@jpn-- that is odd.
|
|
@jpn-- Smoke testing the settings checker with a bad value against the [00:26.24] ERROR: Error checking settings for av_ownership using files av_ownership.yaml: 1 validation error for AVOwnershipSettings
LOGIT_TYPE
Input should be 'MNL' or 'NL' [type=literal_error, input_value='NOT_A_LOGIT', input_type=str]Reading through your test in more detail, I don' think the settings checker is invoked at all. It's not called on a per-step basis. Its done once for all models before an ActivitySim run. https://github.com/ActivitySim/activitysim/blob/e345f9774e14d0eac7fe01b2e22aeca28c2b821f/activitysim/cli/run.py#L388 This does make it hard to test - and there may well be better way to code it in. |
As a follow on from ActivitySim PR 950, this pull request demonstrates a pattern for building a settings checker for extensions.
The key requirements are that users create a checker module in their extensions directory. This must be called
settings_checker.pyand contain a (dict) registry of settings calledEXTENSION_SETTER_CHECKINGS. The general pattern established in core repository should be followed.