Conversation
nneveu
left a comment
There was a problem hiding this comment.
This is looking great! I want to look at the oracle and aida py files a little more, but nice work. I left some minor comments. Also, sorry to nag, maybe add a few comments.
| import slac_db.config | ||
| import slac_db.oracle | ||
|
|
||
| def to_oracle_db(csv_source=None): |
There was a problem hiding this comment.
Some docstrings would be helpful to indicate that this function is where the Oracle db is made, and defaults to lcls_elements.csv
|
This is really great so far. I think docstrings and comments would be very helpful in interpreting the data flow and persistent session mechanics that are happening in this module. |
shamin-slac
left a comment
There was a problem hiding this comment.
Looking good so far! I just had a couple minor comments, and I'll echo the request for more comments/docstrings.
| p = _Parser(csv_source=csv_source) | ||
| return slac_db.oracle.recreate(p) | ||
|
|
||
| class _Parser(): |
There was a problem hiding this comment.
Are the parentheses here to suggest that this takes an argument?
|
|
||
| def recreate(parser): | ||
| assert not _meta | ||
| assert parser.addresses |
There was a problem hiding this comment.
Are these assert statements here for debugging? If not, I think it's better to not use assert statements for these kinds of checks.
There was a problem hiding this comment.
Given the discussion in the workshop, should these be AssertError instead?
There was a problem hiding this comment.
Eloise will add an error message
|
|
||
| def recreate(parser): | ||
| assert not _meta | ||
| assert parser.rows |
There was a problem hiding this comment.
Same comment as earlier
| import slac_db.config | ||
| import slac_db.oracle | ||
|
|
||
| def to_oracle_db(csv_source=None): |
| @@ -0,0 +1,15 @@ | |||
| import slac_db.aida | |||
|
|
|||
| def to_aida_db(): | |||
There was a problem hiding this comment.
rename this to 'directory service'.
|
|
||
| def recreate(parser): | ||
| assert not _meta | ||
| assert parser.addresses |
|
|
||
| def recreate(parser): | ||
| assert not _meta | ||
| assert parser.addresses |
There was a problem hiding this comment.
Eloise will add an error message
Added Oracle and AIDA DBs along with ways to generate them.
Database structure is defined as follows:
slac-db/slac_db/oracle.py
Lines 106 to 126 in de7a98f
uridefines the access method and location of the database.schemadefines the database columns and their data types. This is not a complete list for the Oracle db.An interesting thing to note here is the global
_metavariable defined at the top of the module. I'd never seen this pattern before looking through the slicops db implementation.First, note that
_metais contained in the module's namespace. Code that importsslac_db.oraclecan only access_metawithslac_db.oracle._meta._metatells SQLAlchemy how to access the database, and creates asession. The programmer usingslac_db.oracledoesn't need to know about the_metaobject, so we can hide it as an attribute of the module. Now we don't need adatabase_readerclass to manage database location, format, access method, etc. Everything is defined at the module level._metais only defined once we need a session. It's bad practice to give a lot of overhead to loading a module, particularly when there's a connection to an external resource involved. We only want to encounter a possible connection error once the programmer expects a connection to exist.This particular
sessionconcept is from a wrapper in pykern. I thought about writing my own implementation, but the one Radiasoft provided is very convenient.Copied from #20