-
Notifications
You must be signed in to change notification settings - Fork 129
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
implement composable auspice config file for augur export v2
#298
Comments
Apologies if this isn't the correct place, but here are some issues identified by @jameshadfield and I where we would like different functionality for running with/without a config file. The goal, as I see it, is to tackle one of the most common problems people have: When running tutorials and from ppl trying out on their own, they add a trait to their run, but don't change the config file - then they have no idea why the trait isn't showing up.* We should have a very easy way to run export command-line-only where everything you supply is transparent. It will be less fancy, but it will allow people to easily visualise what they've run. Then, if they want to 'prettify' it, they can explore also including a config file. Any extended config stuff (flu-specific, etc) should sit on top of the 'default' config file which can be supplied, so it's not required. This should make it fairly easy for those familiar with the current config file use to adapt to the new use. I would propose (if possible) the general rule is 'override command-line if present in config file', but that a 'complete' config is not necessary if something has been supplied via command-line. So, in the config I maybe just supply TraitsKeys, Legend, & Menu Items
Note that there is no property for Excluding Traits from Color By Excluding Filter Bys Other notes:
*There was a guy at ABPHM who downloaded augur/auspice ran his own TB data in the time between my talk finishing and the end of the next talk. (Amazing) Even he didn't realise how to modify the config file to make his stuff show up. |
We should probably work to include footer text in the new unified JSON and thus have a way to specify this (perhaps only via config file, since could be large?) in |
I've worked a bit on how to guess 'type' for |
This is all great, @emmahodcroft! Thanks for all the work you've done thinking about this. :-) There's only one thing I'd like to suggest we do differently:
This is the opposite of the Unix conventions for commands which take a config file. The conventional order of overriding things goes like this, from lowest priority to highest priority:
This is often accomplished by building a config data structure for each level (1-4) and then doing a deep merge starting with 1, then overlaying 2, etc. For example, the command-line option processing would build a partial config data structure which would then be merged over levels 1+2. |
Ok, I think this is fine - in this case I'd suggest some messages to remind the user of this - maybe at the end of |
Agreed about the usefulness of warnings like that! |
Yup, that's it! -- https://github.com/nextstrain/augur/blob/master/augur/data/schema.json#L156 This is a great summary of how the command line args & config should interact. @huddlej what are your thoughts on the config file design & composability? If we can agree on something before next wednesday I hope we can implement it as part of #297 |
I completely agree with @tsibley's recommended approach for composable config files and would propose the same thing for this current issue. @jameshadfield and @emmahodcroft you bring up a lot of important details about how the config files need to be structured and each of these likely warrant their own separate issue so they can be addressed by whoever is most able to work on them. I am way behind on the changes to the auspice schema and will need to catch up a little, but I don't think the specifics prevent me from writing up a proposed interface for Can we plan to:
If this plan won't work because command line options to override config settings is tightly coupled to this composable config feature, we could consider moving the command line arguments component of #297 into this issue. My hope generally is that an individual issue can be addressed by one or two people at most by one or two commits, so each of us can take on bite-sized pieces of work that address a bigger feature. |
Looking at the discussion here and in the flu repo, it seems that the consensus option is to allow multiple config files via |
This would be exactly my expectation. The last file passed in takes precedence. And fields passed directly in the command line (like |
Yep, those were my expectations. I do think that the config overwriting should be a deep merge of the data structures, not just a top-level update. |
yes, hadn't thought of nested structures here. something like this should do though, right? |
Yep, the recursive function to do a dictionary merge is often pretty short to write. Many languages have the function in their standard library or in a canonical package already. One option for Python would be deepmerge (though I think we'd want to customize its default append-to-list behaviour). |
augur export v2
I think most of the issues here are resolved by the latest PR on this - but it's a rather long convo so I could be missing something. One decision that is left (from above, and generally) is whether we want to have this 'extra' config option (and/or, do we want this now, in v6) - where more complicated options can be imported in a second config file. We haven't spoken about this much, and I'm afraid I don't have a good idea of what kind of things were meant to be implemented here? If it's just that the second config is very similar to the first but has a few different options, that shouldn't be too hard to implement, I imagine. But was there more? |
As this isn't yet started, let's implement this in a future release (but please reopen this issue if you think it can be implemented now!) |
Please read nextstrain/seasonal-flu#5 for background information.
We need to decide on the config file structure, which can then be built into
augur export
after #297 has been closed.The text was updated successfully, but these errors were encountered: