Skip to content

Refac execute

Vincent Delbar requested to merge refac-execute into master

Since it does not work as expected with execute = False and some things I added since I started to help here, and @remicress talked about going back to a more simpler approach, I looked back at it en felt I needed to clean up some of my mess ^^

Well here it is, going back to execute = True by default, or more precisely what I propose here is to really make this explicitly 'optional' with bool parameter frozen = False which is describe as follow in the docstring :

freeze OTB app in order to use execute() later and avoid blocking process during __init___

If the user actually had to say frozen he should remember he must call execute later. Since this param is actually the same thing as the opposite of execute, if you don't want to keep this, that won't change the code anyway.

So we go back to the default behavior behavior where the execution takes place during __init__ but I believe we need to test this extensively.
Since the finished property, execute_if_needed and PropagateConnectMode are not needed, I removed all of it.

I made parameters a property of every otbObjects, it is a merge of both otb.Application internal GetParameters dict and user's defined parameters dict stored in the _parameters attribute, this fix some issues where App's parameters attribute not up to date with otbApplication object. It is used int the write function.

Between WriteOutput or ExecuteAndWriteOutput, what I found to be working with almost every pipelines is, as Nicolas done lately, Execute then WriteOutput in the execute() func, but ExecuteAndWriteOutput in write().
Now the pipeline should only yield errors during the one write test, regarding BandMath and DynamicConvert with null sizes orfeotoolbox/otb#2290 (closed).. Since this may be possible to fix in OTB, I made it green with the allowed_to_fail trick ;) . The were cases where it fail (not only when in the precise order @nicolasnn mentioned in a comment.

Also it seems that there are no duplicated executions anymore but we need to look closely at the pipeline test log to be sure.

See the first commits for the execute things since in the last commits (is is small ones with explicit messages) there's a big one on docstring works, you will see a big diff with the entire branch. Also removed some useful things like the clear function which was not really working anyway, and the "replace fstring" script since it is useless.
This can be merged in the master branch after the add-function-to-rasterio branch, to avoid merge conflicts.

Edit: is just found the "changes" page does not work with commits I made before creating the MR... 😞 should have done it before pushing my branch...
Edit 2: really sorry since I could not respect what I said last time about pushing too many commits

Fixes: #19 (closed) #28 (closed)

Changelog :

  • move execute to otbObject
  • every otbObjects now expose app parameters as a property
  • removed App's finished property, and func clear()
  • replace App's execute argument with frozen=False by default
  • removed App's pixel_type init argument since this should be done using write() only
  • log (numpy name of) pixel type propagated to outputs, in debug mode
  • docstrings (pydocstyle google convention), now we need type hints !
  • avoid to use the reserved "input" keyword as variable in code or docs, prefer "inp"
  • make __get_output_parameters_keys() private since it is already exposed by App.output_parameters_keys
  • add App's description property to return the OTB App GetDocLongDescription (may be we could do the same with the app help ?)
  • renamed App parameter otb_stdout=True to quiet=False
  • renamed App parameter propagate_pixel_type to preserve_dtype
  • add new otbObject property 'dtype'
Edited by Vincent Delbar

Merge request reports