Ouranosinc / PAVICS-e2e-workflow-tests

Test user-level workflow.
Apache License 2.0
0 stars 2 forks source link

Remove WPS name in output paths #116

Closed aulemahal closed 1 year ago

aulemahal commented 1 year ago

Remove WPS name from output url for notebook testing

This replaces bird-house/finch#273 as a solution for the change in output paths in wps outputs.

The previous solution was breaking finch's CI. pyWPS wants "outputpath" and "outputurl" to match. This means that if "outputurl" has two levels, "outputpath" must have the same second level. But, when developing and testing (both locally and on the CI) "outputpath" is not explicitly set, thus is the default, thus does not match "outputurl"'s second level. The previous solution was patching the notebooks directly when generating them, but nothing of the sort was done in the test suite, which then failed.

The proper way to fix this in the testing is this sanitizing file, thus I am suggesting to take care of the path issue here directly, and only here.

The related rollback in finch will be done in bird-house/finch#272.

tlvu commented 1 year ago

How about the reversed: /wpsoutputsfinch (one level) replace to /wpsoutputs/finch 2 levels to match production.

So the one level will be transparent for dev local env.

This is so we can refresh the notebook using the production Jupyter env also, in addition to using make.

This means we have to be able to set outputurl by make start which I am not sure which config file it is reading right now. If this can not be done then your option is the only working one.

Also have to handle raven as well, in addition to finch and weaver.

I can work on this. But I'll need your help to test the dev workflow.

aulemahal commented 1 year ago

If the outputurl has only 1 level, I think we do not need to change the outputpath. So your solution works here also, but I don't see the benefits as it's the same number of file changes?

I am assuming the production notebook tests are passing through this sanitizing file too? Then is there a reason for the pushed notebooks to fit the production one exactly?

So for example:

This proposition necessitates cleaning up finch's Makefile and this PR.

This proposition necessitates modifying finch's config and Makefile slightly and an modified version of this PR.

En somme, both proposition are equivalent to me. Unless we want the notebooks to have realistic (but not exact) paths in their output. But I don't understand why this would be important.

tlvu commented 1 year ago

Your proposition

* Notebook on finch's docs,  with sed :  http://pavics.ouranos.ca/wpsoutputsfinch/RANDOM/out.nc

The sed from make should still produce /wpsoutputs/finch/RANDOM/out.nc

  * Sanitized: http://WPS_URL/wpsoutputs/PLACEHOLDER/out.nc

All Sanitized will be /wpsoutputs/finch/PLACEHOLDER/out.nc

This proposition necessitates modifying finch's config and Makefile slightly and an modified version of this PR.

Agreed

En somme, both proposition are equivalent to me. Unless we want the notebooks to have realistic (but not exact) paths in their output. But I don't understand why this would be important.

Agreed, the difference is for simple output refresh case I can take directly the output produced by Jenkins nightly and commit them straight. With your proposition, to refresh, I always have to make develop and make start and make refresh-notebooks.

aulemahal commented 1 year ago

Haha, I'm still confused. Why couldn't you commit the output of jenkins directly with my proposition? If they passed jenkins, they would pass the CI tests, no?

But also, I'll go with the proposition that demands the less job in any case.

tlvu commented 1 year ago

But also, I'll go with the proposition that demands the less job in any case.

Exactly. I think I'll just actually try your PR instead of trying to interpret it. Maybe it handles my usecase to straight commit from Jenkins output already so that will be the less work solution :D

tlvu commented 1 year ago

The related rollback in finch will be done in bird-house/finch#272.

There is no rollback required. Leave the sed that append finch/ to wpsouputs/ since it does not break anything and any output refresh directly from Jenkins output will contain that finch/ anyways. Keep it there for consistency when doing make refresh-notebooks.