Making save operations robust

[pavlis]

We had a brief discussion of this topic in our meeting last week, but I have a followup that I think is appropriate here in github.

Recall in our discussion I initially argued a save operator was not black and white (success or fail) but couldn't recreate an example on the spot of an example that was no black just success of failure. In writing the CRUD section of the user manual, however, realized there is a clear situation we need to handle that is not a pure 100% success of failure.

Suppose a user wrote a custom algorithm that altered one of the normalized attributes. To be concrete suppose they estimate a revised location for an earthquake from a moment tensor inversion and naively decide they can just post the new estimate to source_lat, source_lon, source_depth, and source_time.

The topic for discussion is what should happen when this user makes this kind of mistake? A range of actions are:

1. The most rigid answer is abort because the user screwed up. Maybe, but most people would find that annoying and a bit heavy handed.
2. The other end member is to silently ignore the changes. I think that is what we are doing right now because we define these attributes as read only (immutable). That is equally bad as the user will be first confused and then annoyed when the find the data got silently dropped.
3. The right model is in the middle. This particular issue is readily implemented in save_data, I think, if we utilize the modified method of Metadata. It may require a modification of the schema class, but what I think that test would need to do is get a list of all normalized data collection names (currently site, channel, and source). Then it could check through the list returned by the modified method of Metadata for names that begin with our magic "collection_" tag (e.g. source_lat). If any such attributes appear in the attribute list we a least need to log it as an error. What to do for a recovery is a subject more open for discussion here. I would suggest in addition to logging the error such Metadata be saved to the wf collection document with their extended names (e.g. source_depth) and the log message make it clear it is being saved there.

This leaves open the question of what, if anything, a save method should return? The old school way would be this:
-1 if the save failed completely
0 for success
+n where n is the number of nonfatal errors logged during the save

The alternative is something like a tuple or a special dbsave return class. To me a simple integer return is fine, but I am old school.

[wangyinz]

hmmm... the problem actually is that our save_data does not support normalized data at all. Maybe we need to support that anyway, but I don't think we've ever discussed the right behavior of saving normalized data. Things to consider includes, the handling of readonly attributes, metadata with/without a collection_id entry, and insert v.s. update. I think these three factors are not really independent.

Also, another complexity is, if we allow attributes such as source_lat in the wf collection, it will confuse our database reader since it will also read the lat from source and turn that into source_lat in the output data object. I think in current implementation it will overwrite the source_lat from the wf. That could be confusing to the users too.

[wangyinz]

The API looks good to me. I just have some questions that I think need to be clarified before we start implementing the new API. See below:

BUT we probably should have a design that does not preclude a different index if that is possible).

I am not sure how to do that properly. I think right now all of our design has assumed _id as the index to reference a collection. This is also how our database design differs from a relational one. By "different index" are you referring to attributes like net sta chan? In this case, because a unique record is indexed by a series of attributes, we will need some serious work to support that in our current database api.

• nomalize=None should be a python list of collections that readers should require to pull normalized data defined in the schema. If None, the default, no normalization is performed at all. If the list has one component with the world "all" use the schema to define the set of normalized data defined for this schema. Not sure the current schema class would support this, but think it is needed. If we have to do that might also require defining a "standard" list of what would be the most common. Then the default could be "['standard']".

If a user asks for a normalized collection that is not defined in the schema, what should we do? If a normalized collection is requested and defined in the schema but its corresponding collection_id entry is missing in the wf, what should we do? Also, it is still not clear to me when source_lat is defined in wf, and we also read in the normalized source collection, which source_lat should be read, and whether some error message need to be logged. Also, if there is a source_foo defined in wf but there is no foo in the normalized source collection, how can we make sure that the user is not confused thinking foo is in the source collection?

Id data needs to be handled differently. I think we can require that an id must never ever be changed in Metadata but ids can only be set or updated in database operations. Hence, as above if an id appears in the list returned by the Metadata::modified method it should be considered serious error and the writer should kill the object. Writers should always write linking ids in a wf collection or secondary saves will lose their cross references.

Please correct me if I'm wrong, but I think sometimes it make sense to change a _id in the metadata. For example, in an earthquake relocation workflow, users may want to change the source that a waveform is associated with, in which case it can be done by changing the source_id key of that waveform's metadata.

[pavlis]

1. I had independently also concluded we should just insist all normalizations with wf collections be done with objectid model. The reason I concluded that was recalling that that model makes things as generic as possible. We don't want to make things like net,sta,chan,loc and integral part of the system.
2. The set of questions about linking ids. I suggest these:

• Foreach collection in the list of normalization: if the cross referencing id is defined load the data the collection contains. If it it is missing return None in read_data. Ensemble readers should silently drop such data. Same for read_distributed_data. That model would be helpful with large data sets as otherwise one can spend a lot of time figuring out why 10 of out 10 million data have a problem.
• If a field like source_id is defined in the wf collection it should be expected to be replaced by that from the source collection on a read. Writers need to check if any normalized data got altered before trying to save the metadata. If a normalized data attribute has been altered it shold have the key altered to something like "bad_source_lat" and saved that way in the wf collection (I know I said that somewhere else before, but worth repeating). Such errors should also be logged in elog and saved after such repairs are made. Otherwise writers should either just write such data anyway or automatically refuse to save any key containing and "_". Would that be too restictive?
• Hadn't thought of the relocation point, but I think a solution may be to say that the C operation of CRUD will never alter any normalization id attribute. Cross referencing IDs should only be allowed through a force mechanism on th U (Update) functions.

I have more words on his you can examine shortly. I am about to push a branch with a fairly length document on CRUD operations.

[wangyinz]

Thanks for explaining more of the details. I only have some comments and questions. See below:

• Foreach collection in the list of normalization: if the cross referencing id is defined load the data the collection contains. If it it is missing return None in read_data. Ensemble readers should silently drop such data. Same for read_distributed_data. That model would be helpful with large data sets as otherwise one can spend a lot of time figuring out why 10 of out 10 million data have a problem.

Maybe instead of assigning a key None, we should just don't put that key into metadata. Otherwise, it contradict with the design of Metadata::is_defined(string key) method.

The other question I have is on Ensemble and distributed readers. I am not sure exactly how you want the read_data method tell the caller that certain data should be dropped. This can normally done with exception handling, but since we are making read_data permissive on such normalization issue, it won't throw any error. Maybe we can have some extra return values, but it might not be a good idea for read_data to return something other than the requested data object. Or, maybe we just kill the returned object within read_data?

• If a field like source_id is defined in the wf collection it should be expected to be replaced by that from the source collection on a read. Writers need to check if any normalized data got altered before trying to save the metadata. If a normalized data attribute has been altered it shold have the key altered to something like "bad_source_lat" and saved that way in the wf collection (I know I said that somewhere else before, but worth repeating). Such errors should also be logged in elog and saved after such repairs are made. Otherwise writers should either just write such data anyway or automatically refuse to save any key containing and "_". Would that be too restictive?

I guess by saying "check if any normalized data got altered before trying to save the metadata", you really mean readonly attributes. Note that normalization and readonly are two independent properties - normalization does not necessarily mean readonly and vice versa. Then, it seems confusing here to only add the bad_ prefix to the readonly, normalized attributes. What about the ones that are readonly but not normalized? I know that we don't have such condition in our current yaml file. However, supposedly user is able to define their own yaml file that has such an attribute. Are we not gonna support that? I guess the solution is that we either add bad_ prefix to any updates to the readonly attributes or not saving them but only logging them in the elog.

• Hadn't thought of the relocation point, but I think a solution may be to say that the C operation of CRUD will never alter any normalization id attribute. Cross referencing IDs should only be allowed through a force mechanism on th U (Update) functions.

Not quite sure on this comment right now, but I guess you are going to expand on this soon.

[pavlis]

I think we settled the first one in the call today. Returning None is not a good idea ever. Worst case it should return a nearly empty object marked dead. We discussed further points I think you preserved in the meeting notes.

On normalized data, I would emphasize that we should adapt the model that the reader should have an option for the list of collections it should try to normalize. The implications are:

1. Failures to load required normalized data should cause a dead object to be returned. To reduce size it should have no sample data to reduce but if should contain and image of the wf document contents to help the user identify problems. I think such data would end up in graveyard (maybe we called it tombstone) collection.
2. This and other related issues demonstrate the importance of something we have talked about a lot in the past - a wf verification standalone program. We can and should recommend running that program on any wf collection before starting a major job. Designing that program should be the subject of a different discussion thread.

Now to the last paragraph questions:

1. Note that normalization and readonly are two independent properties - normalization does not necessarily mean readonly and vice versa. Then, it seems confusing here to only add the bad prefix to the readonly, normalized attributes. What about the ones that are readonly but not normalized?_ First, I think normalized attributes should be viewed as readonly by default and only with some backdoor approach should allow them to be treated any other way. I think a simple model is that if an attribute is defined as readonly (normalized or just set that way) save it with a stock prefix - let's assume "bad_". Hence, something like "net" defined in wf_Seismogram would be saved as "bad_net" and "net" would not be saved and something like "site_lat" if changed incorrectly would become "bad_site_lat".
2. I know that we don't have such condition in our current yaml file. However, supposedly user is able to define their own yaml file that has such an attribute. Are we not gonna support that? I think you are asking about collsion with "bad_" right? Not sure I can imagine someone coming up with a name that required "bad_" as the first 4 characters of the key, but a fair point. Suggest we just use something more verbose and descriptive that would make such things hard to miss in a dump of document contents. We might use: READONLYERROR_ , MODIFYERROR_ , or something like that - all caps being important to make it stand out.

[haruming]

Thanks for a lot of suggestions that make the save operations robust. But I am confused by one of the suggestions above.

Id data needs to be handled differently. I think we can require that an id must never ever be changed in Metadata but ids can only be set or updated in database operations. Hence, as above if an id appears in the list returned by the Metadata::modified method it should be considered a serious error and the writer should kill the object. Writers should always write linking ids in a wf collection or secondary saves will lose their cross references.

Like Ian said, I think sometimes it makes sense to change a normalized _id field in the metadata. Actually, the read-only value for the normalized _id field is false in our yaml file. If users may want to change the source that a waveform is associated with, they could initialize a new source data so that they could change the source_id key of that waveform's metadata.