CV - ESP Zip Creation

[Rwinstonbnc]

Please include answers to these questions as part of your pull request

In the GitHub webUI, use the Write tab to modify the Markdown text that is part of the pull request. For each question simply place an X inside the square brackets, [X], that represents your answer. Make sure there are no blanks inside the brackets, otherwise MarkDown doesn't render properly. Using the Preview tab while editing this form, you can see the formatted/rendered version of the message.

• Q1: Confirm that you have the right to submit the code that is being contributed. Please consider the origin of your code and confirm you have the appropriate rights to make the submission subject to the Apache 2.0 license that applies to everything in this repository of custom steps. If so, follow the instructions for the Contributor Agreement (which is based on the industry-standard Developer Certificate of Origin (DCO)).
  • Yes, I have the right to submit the contributed code on behalf of myself, my company, or any other owner of the code. I have also attached my signed copy of the DCO to this message.
  • No
• Q2: Confirm that your contribution does not include any personally identifiable information (PII), for example, in any examples used in your README file.
  • My contribution does NOT include PII data
  • My contribution includes PII data
• Q3: Confirm your contribution does not include any encryption or other export-controlled technology.
  • My contribution does NOT contain encryption or other export-controlled technology
  • My contribution includes encryption or other export-controlled technology
    ContributorAgreement.txt

[snlwih]

@Rwinstonbnc , have you seen the zip data step contributed by @suknag?

It seems the zip file you create has some special content beyond the what the Zip data step does and is geared towards usage of the file in ESP, correct? And you use the ODS package in the SAS language to create the zip file, correct?

Btw. your check for the caslib associated with a libref assumes that the name of the libref and the name of the caslib are identical, which does not need to be the case. As part of pull request #133 from your colleague Neela, I provided some suggestions and a code snippet in the comments on how to approach this.

[Rwinstonbnc]

Hey Wilbram,
I have not seen that step, but I'll have a look. I'll see if there are elements of Neil's code that I can use. Or, are is there anything I should particularly look at?

You are correct. The data contained in the zip file is meant to include an ASTORE model, an XML file, and possibly a video file. The folder format and XML follows a format the IoT group requested we use.
Yep, I do believe I used ODS to create the zip package.

I'll fix the libref issue and push the fix in all of the steps I submitted.

[snlwih]

@Rwinstonbnc , haven't been able to successfully run this step yet as it requires successful execution of the CV - Train Models step I guess. But as the 1.3 version of SAS Python package DLPy 1.3 is now available via pypi.org, which was needed by that step, continuing the review of that CV - Train models step is back on my todo list. Still have some additional feedback on the update for the CV - ESP Zip Creation step you posted.

Looked at the code and am happy that it now uses code that checks that the selected table is actually a CAS table where applicable and then report and error when applicable. Great!

• But noticed that your codgen is using %symdel and %sysmacdelete with multiple macro names in a single call in combination with the /nowarn option. This is not supported and result in warnings. Best practices is to not generate warnings when the user has specified all information correctly. Therefore, when cleaning up macros and macro variables, you would need to have a separate call for each macro and for each macro variable Here is a snippet:

%symdel mymacrovar1 /nowarn;
%symdel mymacrovar2 /nowarn;

%sysmacdelete mymacro1 /nowarn;
%sysmacdelete mymacro2 /nowarn;

• Also noticed that release date differs between README and About tab.

Could you fix those items?

[snlwih]

@Rwinstonbnc , the readme talks about step requiring an input table that is created by the "CV - Save Model" step. Is that the right name? Pull request #147 uses a different name "CV - Train Models".

Also could you add this pre-req to the About tab of the step itself and more importantly make it part of the general description at the top of your README and the About tab as this is crucial info for using your step?

Btw. I noticed that when you point the step to a "standard" CAS table, still haven't had time to test the CV - Train Models" step, you get runtime errors as shown below. Would it make sense for your codegen to generate a user-friendly error message when the input table does not have the columns/rows that you are expecting, instead of having your codegen run into these runtime errors? Btw. I can live with postponing the codegen check to a next release if the general description makes it very clear what is expected from the input table.

Here are some examples of those errors:

WARNING: Apparent symbolic reference MAC_IMAGE_WIDTH not resolved.
WARNING: Apparent symbolic reference MAC_NUM_OBJS not resolved.
ERROR 22-322: Syntax error, expecting one of the following: a name, a quoted string, a numeric constant, a datetime constant, 
              a missing value, bitstring, INPUT, PUT.  

[snlwih]

@Rwinstonbnc , really would like to wrap this one up so it can be merged.
As discussed offline, updating the readme/about to reference correct names of related steps is the first priority.
I don't know whether the layout of a model table has a documented structure. But at least your code should not generate errors at runtime when an input table is selected that does not adhere to what you expect. And we should consider SAS warnings about unresolved macro variables an error as well.

If that code check requires more time, then just fix the name references and push that change and we'll merge it. Might then consider the code check a bug that could be reported as a GitHub Issue of type bug, so it isn't forgotten about.

[snlwih]

@Rwinstonbnc , btw. make sure you update the last updated date in both readme and about when making changes to reflect the last date you touched any of the files.

[snlwih]

@Rwinstonbnc , I couldn't resist and quickly looked at your code. Checking that the input table adheres to the structure you need seems to be quite forward. One way to do this is as follows:

1. Check that the required column(s) exist in the input table and stop further processing if it doesn't.
2. Check that all required row values exist in the input table and stop further processing if they don't

Here are some code snippets;

• Check that column exist

  %macro columnExists(datasetName, columnName);
  %* Assumes that datasetName exists;
  %* Returns 1 when columnName exists, otherwise 0;
  %local dsid colnum rcColumnExists;
  %let dsid=%sysfunc(open(&datasetName));
  %let colnum=%sysfunc(varnum(&dsid,&columnName));
  %let rc=%sysfunc(close(&dsid));
  
  %if &colnum>0 %then %let rcColumnExists=1;
                %else %let rcColumnExists=0;
  &rcColumnExists;
  %mend columnExists;
  

  And here is a quick test:

  cas;
  libname casuser cas caslib="casuser";
  data casuser.class; set sashelp.class;
  
  %put Does column age exist?: %columnExists(casuser.class,age);
  %put Does column colDoesNotexist exists: %columnExists(casuser.class,colDoesNotExist);
  

• Check that values exist in specific column
  This quick and dirty code snippet adds found_ variables in your data step. When the last record has been read from the input table, it checks that all required values have been found and otherwise aborts further processing.

  data _null_;
     set &tbleforESP_lib..&tbleforESP_name end=flag_end;
     retain found_model_folder found_model_name .... 0;
  
     if type="model_folder" then do;
       call symputx("mac_project_path", name);
       found_model_folder=1;
     end;
  
     ...
    if flag_end then do;
      /* check that all required row values have been encountered */
      if not(found_model_folder=1 and found_model_name=1 and ...) then do;
         putlog "ERROR: ...";
         abort cancel 42; /* choose a specific number, 42 isn't really the answer to everything :-) */
      end;
    end;
  run;