StudyConfig infrastructure #233
Conversation
| def download_umls_files( | ||
| self, | ||
| target: str = "umls-metathesaurus-mrconso-file", | ||
| target: str = "umls-metathesaurus-full-subset", |
There was a problem hiding this comment.
this is the new target of the UMLS study.
There was a problem hiding this comment.
What did you do with Mr. Conso?!
There was a problem hiding this comment.
Mr. Conso brought all his friends along! He's kind of an inconsiderate guest, I didn't get enough snacks.
| def download_umls_files( | ||
| self, | ||
| target: str = "umls-metathesaurus-mrconso-file", | ||
| target: str = "umls-metathesaurus-full-subset", |
There was a problem hiding this comment.
What did you do with Mr. Conso?!
| self.cursor, self.schema_name, verbose=self.verbose | ||
| self.cursor, self.schema_name, verbose=self.verbose, config=config |
There was a problem hiding this comment.
This is an early comment before I've read the rest of the PR - but looks like you went light-touch with this first cut. Like, you could have removed self.cursor and self.schema_name args here and had run_protected_table_builder grab them from config.db_backend, yeah? And that sort of refactor might be in play in the future?
Not a criticism, just trying to get a handle on the current PR does and what the future vision might be, as I see a lot of similar args that could be removed around the place.
There was a problem hiding this comment.
a future refactor to remove those is the plan, because that is a breaking change to the interface that studies use. Right now, table builders in other studies can just take the new config object and ignore it.
cumulus_library/cli_parser.py
Outdated
| help="An API Key for the UMLS API", | ||
| ) | ||
| build.add_argument( | ||
| "--replace-existing", |
There was a problem hiding this comment.
This naming is also lightly tricky - as a user not steeped in the file uploading feature, I might reasonably think this arg does what clean does - i.e. it refers to whether or not to delete existing tables. I know that's not what you had in mind when naming this arg, but if you could somehow scope this arg down to just file uploads? Maybe something more elegant than --replace-existing-uploads?
Or perversely, maybe going less verbose but in a way that doesn't create confusion with clean - like --force/-f, which is often used for this sort of thing when dealing with file-oriented commands like cp or rm.
There was a problem hiding this comment.
(Though --force may get overloaded once we have two kinds of things we want to force through, so it's not necessarily a good idea, just an idea.)
There was a problem hiding this comment.
this is so weird because it's only affecting a slice of the total state. lemme sleep on it, i'll try to get something a bit more nuanced, but it may just be --replace-existing-uploads - :hopefully: this is developer only, though there are edge cases for users
There was a problem hiding this comment.
--force-uploads splits the difference
|
Nice refactoring - excited to use it 😄 |
This PR adds a StudyConfig object, which transports values from the CLI down to individual studies, for customizing behavior of study runs.
It also contains several tweaks in support of the UMLS study, and moves the upload logic for databases into each database class.
Checklist
docs/) needs to be updated