pacificclimate / climate-explorer-data-prep

0 stars 0 forks source link

update_generate_climos_history in generate_climos.py uses sys.argv to get command line arguments, which is not valid in WPS. #123

Closed sum1lim closed 4 years ago

sum1lim commented 4 years ago

Function update_generate_climos_history is created as a part of PR#117 to resolve issue#82.

Although this works perfectly fine when generate_climos script is run using a terminal command line, it may not be ideal to be utilized in WPS or any other programs since sys.argv is used to create a string for generate_climos history line. For example, wps_generate_climos passes arguments to create_climos without stdin input, and this would cause sys.argv to be an empty list, which makes a history line without any information.

Therefore, I am thinking of modifying the function to create a history line only using parameters passed into create_climos. I would like to get suggestions on which arguments to be included in the history line.

nikola-rados commented 4 years ago

My understanding of the original intention of update_generate_climos_history was to save the method call in history. If that is the case I believe displaying all the params for create_climos sort of does the same thing, although in a way that may be less understandable?

I suppose a way to tell if the method was called through the script or through wps would be to check if sys.argv contains anything? If it does not we can assume we called the method through a process and in that case we display some of the params in an understandable manner.

sum1lim commented 4 years ago

I am sorry for assigning everyone. I was confused with reviewers in PR. @nikola-rados that is a good suggestion to keep sys.argv if it is not empty. However, I am not sure which parameters to include in the history for the case when sys.argv is empty. Some parameters have default values if they are not given as an input. Although, I think it is OK to include every parameter even for default values.

nikola-rados commented 4 years ago

I'm not sure how much detail would be necessary but I feel as though all of the params would be too much. Perhaps just any that were changed from their default values could be included? @jameshiebert do you have any thoughts?

jameshiebert commented 4 years ago

Yeah, I have a few scattered thoughts:

  1. @sum1lim You have definitely identified a hole in our thinking with respect to the use of sys.argv. While it was convenient and simple for command line usage, now that we have implemented a different usage, we are running up against the limitations of using it.
  2. I think "displaying" the parameters (or more specifically, encoding them into a "history" entry) is a reasonable approach.
  3. I don't think that including even default values is too much. I.e. I think including the defaults is reasonable.
  4. Not saying that this is a good idea... not even sure if I would do this... but if you assume that sys.argv is actually writable, you could imagine a possible hack that leverages that. In fact it is:
>>> import sys
>>> sys.argv
['']
>>> sys.argv = ['foo', 'bar']
>>> sys.argv
['foo', 'bar']