-
Notifications
You must be signed in to change notification settings - Fork 7
Improve load_astrodb #192
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
Improve load_astrodb #192
Conversation
I think this is in good shape but would like to write some tests. |
I think we should move the reference_tables out of |
@dr-rodriguez agrees we should rename it to LOOKUP_TABLES. Which can be done this PR for the |
make more sub functions, eg., for loading the LOOKUP_TABLES variable. |
spec = importlib.util.spec_from_file_location(db_name, init_path) | ||
db_module = importlib.util.module_from_spec(spec) | ||
spec.loader.exec_module(db_module) | ||
REFERENCE_TABLES = db_module.REFERENCE_TABLES |
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.
We need to add some message (logger.info or even warning) that alerts the user this was one on their behalf.
return db | ||
|
||
|
||
def _rebuild_db(db_path, db_file, data_path, reference_tables, felis_schema): |
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.
May want to set felis_schema = None
here to avoid issues with line 122 below (which checks whether it's None or not)
Would be nice if the load_astrodb function name made it more obvious that this function 1) reads from JSON files if recreatedb = True and 2) creates a sqlite file. Typing this made me realize that we need two different functions.
|
while chatting with David: json_path, felis schema, reference table paths should be optional parameters that have defaults that match how the template repo is organized.
|
We could consider using configparser for how to build up a config file: https://docs.python.org/3/library/configparser.html |
[database settings]
lookup_tables = ["Publications, ..."]
db_data_path = "data"
felis_path = "schema/felis.yaml"
db_name = "name.sqlite"
load in with configparser or tomllib. https://docs.python.org/3/library/tomllib.html#module-tomllib |
Improve the
load_astrodb
function to look for reference_tables andfelis_schema
where we expect them to be. This should reduce the complexity of ingest scripts and takes advantage of the standarization provided by the template.Inspired by SIMPLE-AstroDB/SIMPLE-db#627