Error handling in cmor_CV.c

[ehogan]

Some of the functions in the cmor_CV.c file return a value of -1 when an error is encountered, while others return a value of 1.

This causes problems at this point in the cmor.c file, since the return values from each call to the cmor_CV_check* functions are added together. Depending on the combination of issues with the CVs in the cmor_dataset JSON file, this can result in the returned ierr being 0.

Would it be possible to use a consistent return value (i.e. 1 so the ierr used in the error message indicates the number of errors) when an error is encountered for the functions in cmor_CV.c file, please?

[dnadeau4]

I used negative number to count the error message. @doutriaux1 used 0 or 1 for False and True in other functions.

I could not find any return values that would affect the computation error messages.

Can you tell me how you encountered this issue?

[ehogan]

My apologies @dnadeau4, I misunderstood the problem. The issue is due to the fact that many of the checks performed in cmor_CV_checkParentExpID do not return an error code (most of the loops that end with cmor_handle_error(msg, CMOR_CRITICAL); are not followed by return (-1);). This has the same result as described in my initial comment (the returned ierr equals 0).

[taylor13]

Do you think that when a cmor_handle_error is indicates an error has been encountered that ierr should be set to -1 or +1, rather than 0? Would that be the right thing to do? Is it possible?

[ehogan]

@taylor13, I'm not sure if your question was directed at me, but the work I did for #171 involved recording (via cmor_handle_error_var) when a critical error (CMOR_CRITICAL specifically, rather than just a call to cmor_handle_error) was encountered so that cmor_close knew whether to finalise the file or not. In this case, cmor_handle_error_var is called on L4309 after calling cmor_setGblAttr, but also after ierr has been appended to (by cmor_CreateFromTemplate). Therefore, in this case, it makes sense for cmor_setGblAttr and cmor_CreateFromTemplate to both return either positive or negative values for ierr (otherwise they could cancel each other out).

I hope that's helpful? :)

[dnadeau4]

The program sends a SIGTERM and the number of error ierr is lost when it is killed. At this stage, I don't want to make such a major code change. I called 455 cmor_handle_error() functions in all the .c.

I only work 10%-20% of my time for PCMDI now until next year.

[taylor13]

Can we leave this open as a suggested future change? No need to address this issue, but we might want to in the future.

[ehogan]

My apologies for not being clear.

I believe there is a genuine issue here.

In some cases where there is a problem in cmor_CV.c, i.e. cmor_handle_error is called, a negative value is not returned (see my original comment). See here, here and here (although should these two be a CMOR_WARNING?), here (and many others in this function) and here. This can be solved by adding return (-1); after every cmor_handle_error call (excluding those that are just warnings). I can add a pull request for this next week, if that's helpful?

Also, this and this is just a warning, so I don't think -1 should be returned.

I have convinced myself that the functions called in cmor_write before the call to cmor_handle_error_var here have negative return values, so the issue I reported in the last sentence here is not an issue :)

[dnadeau4]

Pushing to 3.3.1, cannot be fix in 3.3.0 release time frame.

[dnadeau4]

@ehogan I have very confused with all this #245 (comment) Some of the place where you want to return(-1) will stop the loop and you won't get all the information like a compiler (PCMDI requirements).
here

I will push this to 3.3.2, but I am not sure I can work on this, since I only work 8 hour/week for PCMDI. If you can create a PR, I might be able to merge it if my tests do not fails.

[dnadeau4]

@ehogan @jonseddon just to let you know that @doutriaux1 is now working on CMOR3. How are you doing with CMIP6? I have not heard from you in a long time.

[mauzey1]

This issue is similar to what I observed in #540.

Should we have all of the CMOR CV functions return a positive error count rather than just -1?

[taylor13]

It would be really nice to hear why some errors are "-1" and others "1". There must have been a good reason for making the distinction. Anyone remember one? @dnadeau4? @doutriaux1?

[doutriaux1]

@taylor13 , @dnadeau4 commented on this at: #540

[mauzey1]

A lot of the functions in cmor_CV.c get called in cmor_setGblAttr.

cmor/Src/cmor.c

Lines 2963 to 2989 in 9511753

	if (cmor_has_cur_dataset_attribute(GLOBAL_ATT_INSTITUTION_ID) == 0) {
	ierr += cmor_CV_setInstitution(cmor_tables[nVarRefTblID].CV);
	}
	if (cmor_has_cur_dataset_attribute(GLOBAL_IS_CMIP6) == 0) {
	ierr += cmor_CV_checkSourceID(cmor_tables[nVarRefTblID].CV);
	ierr += cmor_CV_checkExperiment(cmor_tables[nVarRefTblID].CV);
	ierr += cmor_CV_checkFurtherInfoURL(nVarRefTblID);
	ierr += cmor_CV_checkGrids(cmor_tables[nVarRefTblID].CV);
	ierr += cmor_CV_checkParentExpID(cmor_tables[nVarRefTblID].CV);
	ierr += cmor_CV_checkSubExpID(cmor_tables[nVarRefTblID].CV);
	}
	//
	// Set user defined attributes and explicit {} sets.
	//
	ierr += cmor_CV_checkGblAttributes(cmor_tables[nVarRefTblID].CV);
	//
	// Copy block to ensure all attributes are set for obs4MIPs
	// especially (source_label)
	//
	if ( cmor_current_dataset.furtherinfourl[0] != '\0') {
	ierr += cmor_CV_checkSourceID(cmor_tables[nVarRefTblID].CV);
	ierr += cmor_CV_checkFurtherInfoURL(nVarRefTblID);
	}
	ierr += cmor_CV_checkISOTime(GLOBAL_ATT_CREATION_DATE);

cmor_setGblAttr accumulates these errors and returns them in cmor_write, which then accumulates them with other errors for this test:

cmor/Src/cmor.c

Lines 4476 to 4511 in 9511753

	ierr += cmor_setGblAttr(var_id);
	/* -------------------------------------------------------------------- */
	/* Figures out path */
	/* -------------------------------------------------------------------- */
	strncpy(szPathTemplate, cmor_current_dataset.path_template,
	CMOR_MAX_STRING);
	/* -------------------------------------------------------------------- */
	/* Add outpath prefix if exist. */
	/* -------------------------------------------------------------------- */
	strncpytrim(outname, cmor_current_dataset.outpath, CMOR_MAX_STRING);
	/* -------------------------------------------------------------------- */
	/* Make sure last character is '/'. */
	/* -------------------------------------------------------------------- */
	if ((strlen(outname) > 0) && (outname[strlen(outname)] != '/')) {
	strncat(outname, "/", CMOR_MAX_STRING);
	}
	if (CMOR_CREATE_SUBDIRECTORIES == 1) {
	ierr +=
	cmor_CreateFromTemplate(nVarRefTblID, szPathTemplate, outname,
	"/");
	} else {
	ierr +=
	cmor_CreateFromTemplate(nVarRefTblID, szPathTemplate, msg, "/");
	}
	if (ierr != 0) {
	sprintf(ctmp,
	"Cannot continue until you fix the errors listed above: %d",
	ierr);
	cmor_handle_error_var(ctmp, CMOR_CRITICAL, var_id);
	cmor_pop_traceback();
	return (1);
	}

It doesn't distinguish between positive and negative errors, just nonzero. This works fine as long as all the functions return negative integers for errors, which all of the functions in the above example do return.

However, if you were to mix up positive and negative errors you could wind up making it look like there were no errors. That was one of the issues I encountered while working with PrePARE in #540 where negative errors from cmip6_cv functions were being clobbered by PrePARE's checks where it added 1 to the error count. I resolved that issue by just detecting if the error is nonzero and then adding 1 to the error count.

cmor/LibCV/PrePARE/PrePARE.py

Lines 492 to 493 in 9511753

	if cmip6_cv.check_parentExpID(table) != 0:
	self.errors += 1

There was one place that returned a positive integer for a warning in cmor_CV.c.

cmor/Src/cmor_CV.c

Lines 656 to 664 in 9511753

	if(CV_source_id->nbObjects == -1) {
	snprintf(msg, CMOR_MAX_STRING,
	"You did not define a %s section in your source_id %s.\n! \n! \n! "
	"See Control Vocabulary JSON file. (%s)\n! ",
	CV_KEY_SOURCE_LABEL, szSource_ID, CV_Filename);
	cmor_handle_error(msg, CMOR_WARNING);
	return(1);
	break;
	}

Should the above check be for an error instead of a warning?

Another interesting thing to note of cmor_write is that there are errors that are -1.

cmor/Src/cmor.c

Lines 4410 to 4434 in 9511753

	if (var_id > cmor_nvars) {
	cmor_handle_error("var_id %i not defined", CMOR_CRITICAL);
	cmor_pop_traceback();
	return (-1);
	};
	/* -------------------------------------------------------------------- */
	/* Make sure that time_vals (and possibly time_bounds) are passed */
	/* when CMOR is running in append mode. */
	/* -------------------------------------------------------------------- */
	if (bAppendMode) {
	size_t refTableID = cmor_vars[var_id].ref_table_id;
	size_t refAxisID = cmor_axes[cmor_vars[var_id].axes_ids[0]].ref_axis_id;
	if ( time_vals == NULL ||
	( cmor_tables[refTableID].axes[refAxisID].must_have_bounds == 1 &&
	time_bounds == NULL ) ) {
	cmor_handle_error("time_vals and time_bounds must be passed through cmor_write "
	"when in append mode",
	CMOR_CRITICAL);
	cmor_pop_traceback();
	return (-1);
	};
	};

However, other errors in cmor_write are 1.

cmor/Src/cmor.c

Lines 4504 to 4543 in 9511753

	if (ierr != 0) {
	sprintf(ctmp,
	"Cannot continue until you fix the errors listed above: %d",
	ierr);
	cmor_handle_error_var(ctmp, CMOR_CRITICAL, var_id);
	cmor_pop_traceback();
	return (1);
	}
	ierr = cmor_mkdir(outname);
	if ((ierr != 0) && (errno != EEXIST)) {
	sprintf(ctmp,
	"creating outpath: %s, for variable %s (table: %s). "
	"Not enough permission?",
	outname, cmor_vars[var_id].id,
	cmor_tables[cmor_vars[var_id].ref_table_id].szTable_id);
	cmor_handle_error_var(ctmp, CMOR_CRITICAL, var_id);
	cmor_pop_traceback();
	return (1);
	}
	strncat(outname, "/", CMOR_MAX_STRING - strlen(outname));
	/* -------------------------------------------------------------------- */
	/* Verify that var name does not contain "_" or "-" */
	/* -------------------------------------------------------------------- */
	for (i = 0; i < strlen(cmor_vars[var_id].id); i++) {
	if ((cmor_vars[var_id].id[i] == '_') ||
	(cmor_vars[var_id].id[i] == '-')) {
	snprintf(outname, CMOR_MAX_STRING,
	"var_id cannot contain %c you passed: %s "
	"(table: %s). Please check your input tables\n! ",
	cmor_vars[var_id].id[i], cmor_vars[var_id].id,
	cmor_tables[nVarRefTblID].szTable_id);
	cmor_handle_error_var(outname, CMOR_CRITICAL, var_id);
	cmor_pop_traceback();
	return (1);
	}
	}

The documentation just says that cmor_write returns 0 when successful.
https://cmor.llnl.gov/mydoc_cmor3_api/#cmor_write

There's a statement in cmor_tables.c that is checking if cmor_CV_set_entry returns 1.

cmor/Src/cmor_tables.c

Lines 871 to 874 in 9511753

	if (cmor_CV_set_entry(&cmor_tables[cmor_ntables], value) == 1) {
	cmor_pop_traceback();
	return (TABLE_ERROR);
	}

However, cmor_CV_set_entry doesn't seem to return anything aside from 0.

cmor/Src/cmor_CV.c

Lines 2228 to 2285 in 9511753

	int cmor_CV_set_entry(cmor_table_t * table, json_object * value)
	{
	extern int cmor_ntables;
	int nCVId;
	int nbObjects = 0;
	cmor_CV_def_t *CV;
	cmor_CV_def_t *newCV;
	cmor_table_t *cmor_table;
	cmor_table = &cmor_tables[cmor_ntables];
	cmor_is_setup();
	cmor_add_traceback("_CV_set_entry");
	/* -------------------------------------------------------------------- */
	/* CV 0 contains number of objects */
	/* -------------------------------------------------------------------- */
	nbObjects++;
	newCV = (cmor_CV_def_t *) realloc(cmor_table->CV, sizeof(cmor_CV_def_t));
	cmor_table->CV = newCV;
	CV = newCV;
	cmor_CV_init(CV, cmor_ntables);
	cmor_table->CV->nbObjects = nbObjects;
	/* -------------------------------------------------------------------- */
	/* Add all values and dictionaries to the M-tree */
	/* */
	/* { { "a":[] }, {"a":""}, { "a":1 }, "a":"3", "b":"value" }.... */
	/* */
	/* Cv->objects->objects->list */
	/* ->objects->string */
	/* ->objects->integer */
	/* ->integer */
	/* ->string */
	/* ->list */
	/* -------------------------------------------------------------------- */
	json_object_object_foreach(value, CVName, CVValue) {
	nbObjects++;
	newCV = (cmor_CV_def_t *) realloc(cmor_table->CV,
	nbObjects * sizeof(cmor_CV_def_t));
	cmor_table->CV = newCV;
	nCVId = cmor_table->CV->nbObjects;
	CV = &cmor_table->CV[nCVId];
	cmor_CV_init(CV, cmor_ntables);
	cmor_table->CV->nbObjects++;
	if (CVName[0] == '#') {
	continue;
	}
	cmor_CV_set_att(CV, CVName, CVValue);
	}
	CV = &cmor_table->CV[0];
	CV->nbObjects = nbObjects;
	cmor_pop_traceback();
	return (0);
	}

[mauzey1]

I think to apply the "keep going like a compiler and accumulate the number of errors" method of CMOR, functions like cmor_CV_checkParentExpID should have an error count that is incremented at every error encountered and returned at the end. Several of the errors in cmor_CV_checkParentExpID are CMOR_CRITICAL. Shouldn't they be CMOR_NORMAL to prevent the error checking from ending prematurely?

[mauzey1]

@durack1 @taylor13
I have some questions about this section in cmor_CV_checkParentExpID.

cmor/Src/cmor_CV.c

Lines 1045 to 1063 in 9511753

	// Do we have a parent_experiment_id?
	if (cmor_has_cur_dataset_attribute(GLOBAL_ATT_PARENT_EXPT_ID) != 0) {
	CV_parent_exp_id = cmor_CV_search_child_key(CV_experiment,
	PARENT_ACTIVITY_ID);
	if (CV_IsStringInArray(CV_parent_exp_id, NO_PARENT)) {
	cmor_pop_traceback();
	return (0);
	} else {
	snprintf(msg, CMOR_MAX_STRING,
	"Your input attribute \"%s\" is not defined properly \n! "
	"for your experiment \"%s\"\n!\n! "
	"See Control Vocabulary JSON file.(%s)\n! ",
	GLOBAL_ATT_PARENT_EXPT_ID, CV_experiment->key,
	CV_Filename);
	cmor_handle_error(msg, CMOR_CRITICAL);
	cmor_pop_traceback();
	return (-1);
	}
	}

On line 1048, shouldn't that be PARENT_EXPERIMENT_ID rather than PARENT_ACTIVITY_ID?

Lines 1049-1051 will make the function return 0 if "no parent" is found in the above attribute from CMIP6_CV.json. Does that mean we can ignore checking the other attributes in cmor_CV_checkParentExpID when the CV has "no parent" for parent_experiment_id? Shouldn't it just report an error if it doesn't find "no parent" in the CV, and then continue finding other errors?

[durack1]

@mauzey1 there are two identifiers, the parent_activity_id (or MIP, e.g. CMIP, DAMIP, AerChemMIP etc) and the parent_experiment_id so for e.g. the mip_era / activity_id / experiment_id: CMIP6 CMIP historical experiment would normally have a parent experiment of CMIP6 CMIP piControl

[mauzey1]

@durack1
What I was pointing out in #245 (comment) was that it was getting the parent_activity_id from the experiment's CV rather than the parent_experiment_id when seeing if it was "no parent" when there is no parent_experiment_id in the data file.

If there is no parent_experiment_id attribute in the file and "no parent" was in the parent_experiment_id of the experiment's CV, then it would exit as successful. Does that mean a file having no parent_experiment_id attribute is treated as having parent_experiment_id = "no parent" if "no parent" was one of the parent experiments in the experiment's CV? If so, then shouldn't the other attributes be evaluated as if it was the "no parent" case? Otherwise, shouldn't having no parent_experiment_id in the data file be considered an error regardless of what the parent is in the experiment's CV?

[durack1]

@mauzey1 got it, this logic was something that @taylor13 likely worked on with @dnadeau4 . So I might defer to @taylor13. Having said that, there was a CV cleanup a long while back that attempted to address these issues, If we missed some entries, we could run another CV cleanup too

[mauzey1]

@durack1 @taylor13

cmor/Src/cmor_CV.c

Lines 656 to 664 in 7b59863

	if(CV_source_id->nbObjects == -1) {
	snprintf(msg, CMOR_MAX_STRING,
	"You did not define a %s section in your source_id %s.\n! \n! \n! "
	"See Control Vocabulary JSON file. (%s)\n! ",
	CV_KEY_SOURCE_LABEL, szSource_ID, CV_Filename);
	cmor_handle_error(msg, CMOR_WARNING);
	return(1);
	break;
	}

Above are lines from the function cmor_CV_checkSourceID in cmor_CV.c. They check whether a "source_id" object in the CV file has a "source" section. If the number of objects in the "source_id" object is -1, then it will give the warning message saying that the "source" section is not defined. However, this part will return 1 even though it is a warning rather than a error. The next lines of code will search through the "source_id" object to find the "source" section, which would give a warning message and break from the "source_id" section search loop if "source" is not found.

Should the above section of code be changed so that it breaks from the loop but not return? The break is already there but is not reached due to the return. Or should the message be an error instead of a warning?

[durack1]

@mauzey1 just to confirm this is using the cmip6-cmor-tables/Tables/CMIP6_CV.json file isn't it?

I ask, as for this file we do the work of accumulating contributions that are registered in CMIP6_CVs/CMIP6_source_id.json and then create each of the source entries that you are referring to

[mauzey1]

@durack1 Yes, that is the file I am referring to.

[durack1]

@mauzey1 so as we create this file, it will always have a source section, I am uncertain if there are any criteria set in the generation of this (it's the cronjob that you have reconfigured)? If there are tests in how source is constructed, then that should be considered in changing the behavior as described above

[mauzey1]

@durack1 The section I mentioned would only be reached if the "source_id" was not a valid JSON object in cmor_CV.json. For example, if you were processing a file with PrePARE with a "source_id" of "PCMDI-test-1-0"

        "source_id":{
            ...
            "PCMDI-test-1-0": "",
            ...

Should that be considered a critical error since there is something wrong with the table files?

[durack1]

@mauzey1 as we are generating the CMIP6_CV.json file, and control all the content, the only time that this test (and the result) would be useful is in the case that a user has "hacked" the CV file, and included a json-invalid entry.

I think in the case that anything is found wrong with the file(s), then a fatal error should be thrown and the user alerted to the error that was encountered. I make this statement knowing that you have implemented comprehensive testing, so a false positive fatal error is incredibly unlikely.

[mauzey1]

@taylor13 Would you like to comment on the logic of the code section posted in #245 (comment)?

If the data file doesn't have the attribute parent_experiment_id but the experiment in the CV file has "no_parent" as the parent_experiment_id, does that mean we can ignore the other attributes checked by cmor_CV_checkParentExpID? Should that be an error instead?

[taylor13]

Apologies for the delay getting to this. Before commenting on what you asked me to comment on, I agree with @durack1 regarding the check on the source information in the CV.

Now for experiment_id and parent, etc. First, I'll outline how I would have constructed the logic:

Given an experiment_id, the experiment_id CV specifies the only acceptable options for parent_experiment_id and the only acceptable options for parent_activity_id. We need to check that what the user specifies for parent_experiment_id and parent_activity_id is consistent with those options.

If one of the options in the CV is for parent_experiment_id and parent_activity_id to be "no parent", then the user may specify "" for the values, but CMOR should change the empty strings to "no parent" before writing the file.

If the parent_experiment_id != "no parent" (and != ""), then the user must specify the following attributes (but the values are just examples):

    "parent_activity_id":      "CMIP"
    "parent_source_id":       "PCMDI-test-1-0",
    "parent_variant_label":   "r3i1p1f1",
 
    "parent_time_units":      "days since 1850-01-01",
    "branch_method":          "standard",
    "branch_time_in_child":   59400.0,
    "branch_time_in_parent":  59400.0,

If the user fails to specify these attributes, a fatal error should be raised.

Also when parent_experiment_id != "no parent", CMOR should set parent_mip_era = "CMIP6" . (It's probably o.k. for CMOR to invariably set parent_mip_era="CMIP6").

If parent_experiment_id = "no parent", then the first 5 entries should either be set to "" or "no parent"; if the user has specified something else, we should raise an error. (or we could simply override what the user specifies with "no parent" and raise a warning telling the user we have done that.) If the user has specified "", CMOR should replace that with "no parent" before writing the file.

If parent_experiment_id = "no parent", then the last two entries above (the times) may or may not be specified by the user. Unless the user specifies a value, we should omit these two attributes for the "no parent" case. If the user does specify values, they should be recorded in the file.

It is likely that currently CMOR is less thorough than outlined above, but CMOR should be at least consistent with the above. Perhaps @mauzey1 can say whether there are obvious inconsistencies or things that are unclear before I try to address the specifics.

thanks for attempting to address this issue raised so long ago.

[mauzey1]

@taylor13 So if the file has parent_experiment_id = "" or doesn't have parent_experiment_id in global attributes, then we should replace it with parent_experiment_id = "no parent" if that is the valid parent experiment in the CV?

[taylor13]

The CV will never have ""; it will have "no parent" if there is no parent experiment. I think the user is allowed to specify for parent "", if there is "no parent", but CMOR should probably write out the attribute with the value "no parent". I guess if CMOR currently just omits the attribute in this case, that would be o.k., but I don't think CMOR should ever specify "" for the value of an attribute written to a file. That seems difficult for a user to interpret. Do you agree?

[doutriaux1]

Darn I miss these @taylor13 discussions. 😄

[mauzey1]

@taylor13 Here is a change to the code that will replace "" with "no parent" for the parent_experiment_id value. Instead of exiting, it would continue to the next part of cmor_CV_checkParentExpID where it will process the other attributes for the "no parent" case.

    // Do we have a parent_experiment_id?
    if (cmor_has_cur_dataset_attribute(GLOBAL_ATT_PARENT_EXPT_ID) != 0) {
        CV_parent_exp_id = cmor_CV_search_child_key(CV_experiment,
                                                    PARENT_EXPERIMENT_ID);
        if (CV_IsStringInArray(CV_parent_exp_id, NO_PARENT)) {
            snprintf(msg, CMOR_MAX_STRING,
                    "Your input attribute \"%s\" defined as \"\" "
                    "will be replaced with \n! "
                    "\"%s\" as defined in your Control Vocabulary file.\n! ",
                    PARENT_EXPERIMENT_ID, NO_PARENT, NO_PARENT);
            cmor_handle_error(msg, CMOR_WARNING);
            cmor_set_cur_dataset_attribute_internal(PARENT_EXPERIMENT_ID,
                                                    NO_PARENT, 1);
        } else {
            snprintf(msg, CMOR_MAX_STRING,
                     "Your input attribute \"%s\" is not defined properly \n! "
                     "for your experiment \"%s\"\n!\n! "
                     "See Control Vocabulary JSON file.(%s)\n! ",
                     GLOBAL_ATT_PARENT_EXPT_ID, CV_experiment->key,
                     CV_Filename);
            cmor_handle_error(msg, CMOR_NORMAL);
            cmor_pop_traceback();
            return (-1);
        }
    }

[durack1]

@taylor13 @mauzey1 (and why the hell not @doutriaux1) you might want to double check (CMIP6_experiment_id.html)[https://wcrp-cmip.github.io/CMIP6_CVs/docs/CMIP6_experiment_id.html] (or the source json file) to make sure that this assumption holds. I think we cleaned it up, but not 100% sure