pepkit / looper

A job submitter for Portable Encapsulated Projects
http://looper.databio.org
BSD 2-Clause "Simplified" License
20 stars 7 forks source link

Is the path variable still a thing? #322

Closed nsheff closed 1 year ago

nsheff commented 3 years ago

Here, we introduce the path key in the pipeline interface.

http://looper.databio.org/en/latest/writing-a-pipeline-interface/

image

Are these docs outdated? I seem to recall that we switched from this to use var_templates

There are very few docs related to this and I think some of these are wrong...

aaron-gu commented 3 years ago

This is working for me in the hello looper repository as well as a project I created separately. (looper v1.3.0)

donaldcampbelljr commented 1 year ago

Currently, in v1.4.0, one can replace path: count_lines.sh

with

image

or use: image

However, one page of our documentation instructs to use: image

image

However, this does not execute the pipeline script as expected. Looper debug output:

`DEBU 14:11:53 | looper.utils:utils:207 > rendered arg str: {looper.piface_dir}/pipeline/count_lines.sh data/frog2_data.txt --output-parent /home/drc/hello_looper_results/results_pipeline/frog_2``

vs executing the pipeline .sh as expected:

looper.utils:utils:207 > rendered arg str: pipeline/count_lines.sh data/frog2_data.txt

It appears that in the 1st case, rendered arg str:shows {looper.piface_dir} is not being replaced with the correct file path to call the pipeline script.

Also, using the example from the documentation page, namespace pipelines also shows incorrect path, e.g. pipeline/pipeline/count_lines.sh vs pipeline/count_lines.sh

donaldcampbelljr commented 1 year ago

Downgrading to Looper 1.3.2 and using Python 3.8:

Still not calling the script due to an 'extra' "pipeline" in the called path: image

image

However, if one uses {looper.piface_dir}/count_lines.sh vs {looper.piface_dir}/pipeline/count_lines.sh, it will call the script properly: image

image

Submission for an example sample using Looper 1.3.2: image

Submission for an example sample using the same config files but using Looper 1.4.0: image

donaldcampbelljr commented 1 year ago

Additional information, confirmed that:

Command template arguments that also contain {} will not parse correctly, e.g. {pipeline.var_templates.pipeline} ------> points to ------> '{looper.piface_dir}/count_lines.sh'

Not rendered appropriately: Screenshot from 2023-04-05 11-25-47

Changing the config file to have all '{}' commands within command template will rendered the proper script path and execute the pipeline as expected: Screenshot from 2023-04-05 11-37-36

I believe this occurs in function: def jinja_render_template_strictly.

donaldcampbelljr commented 1 year ago

Added check in utils.py to look for nested commands: f3ae33f4f6ed88ff6569b52c225b05035c6d7d42

Next Steps:

  1. Update Looper Documentation to show examples using var_templates/pipeline: https://looper.databio.org/en/latest/writing-a-pipeline-interface/
  2. Note that path variable will be deprecated.
  3. Update hello_world repository to include var_templates (instead of simple path variable).
donaldcampbelljr commented 1 year ago

Per discussion, a different approach for the nested commands was investigated. Instead of checking for nested commands, the program can return the rendered 'var_templates' back to the 'pipeline' namespace.

Proposed change in conductor.py, after rendering var_templates:

            pl_iface[VAR_TEMPL_KEY] = self.pl_iface.render_var_templates(
                namespaces=namespaces
            )
            _LOGGER.debug(f"namespace pipelines: { pl_iface }")

            namespaces["pipeline"]["var_templates"] = pl_iface[VAR_TEMPL_KEY]

This will allow the the proper rendering of the following command_template as it uses the pipeline namespace for rendering NOT pl_iface:


pipeline_name: count_lines
pipeline_type: sample
output_schema: output_schema.yaml
var_templates:
  pipeline: '{looper.piface_dir}/count_lines.sh'
command_template: >
  {pipeline.var_templates.pipeline} {sample.file} --output-parent `{looper.sample_output_folder}
donaldcampbelljr commented 1 year ago

This is now fixed with https://github.com/pepkit/looper/commit/79fef3e68926942d452753adddad07ece962b525

nsheff commented 1 year ago

excellent -- it would probably be best in this case to add add one small pytest to check for this so we don't have a reversion