Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutually exclusive --processes and --process-levels #24

Closed
soxofaan opened this issue Jan 16, 2024 · 10 comments · Fixed by #37
Closed

Mutually exclusive --processes and --process-levels #24

soxofaan opened this issue Jan 16, 2024 · 10 comments · Fixed by #37

Comments

@soxofaan
Copy link
Member

parser.addoption(
"--process-levels",
action="store",
default="",
help="The openEO process profiles you want to test against, e.g. 'L1,L2,L2A'. Mutually exclusive with --processes.",
)
parser.addoption(
"--processes",
action="store",
default="",
help="The openEO processes you want to test against, e.g. 'apply,reduce_dimension'. Mutually exclusive with --process-levels.",
)

The help strings of --processes and --process-levels state that both are mutually exclusive, but I wonder if it is necessary to specify that. The current implementation allows to pass them both at the same time and will just take the intersection of both subsets, which is fine and intuitive I think.

@m-mohr

@m-mohr
Copy link
Member

m-mohr commented Jan 17, 2024

I'm not sure what specifying both would mean and to remove that ambiguity I just stated that they are exclusive, also to make it a bit simpler for others to follow this.

For example, what would providing e.g. L1 and run_udf mean? Does this test no process or L1 + run_udf? I'm not sure what would be expected...

@soxofaan
Copy link
Member Author

To give some background: I'm trying to unify the logic/behavior between the different WPs a bit and these --processes/--process-levels options are a bit confusing in that regard.
The docs state they are mutually exclusive, but as a user you can still provide both and something will happen (if they are truly mutually exclusive I would expect an error).
Additionally, the --processes option is only used in WP5-individual-process testing, the WP6-workflow tests only look at --process-levels.

In the end, both options are just about process selection and I'm wondering if it wouldn't be easier to just provide a single option along the lines of:

  • not specified: cover all processes
  • --processes=L1: only cover L1 processes (i.e. skip or even ignore tests that involve other processes)
  • --processes=min,max: only cover processes min and max
  • --processes=L1,load_stac: cover the L1 processes and load_stac

@m-mohr
Copy link
Member

m-mohr commented Jan 17, 2024

I'm not against unifying the parameters if it's feasible to detect whether something is a process or a level...

With regards to "not specified: cover all processes" please also note that experimental processes are not tested by default.

@clausmichele
Copy link
Member

@soxofaan fine on my side to have a single parameter and simplify the call

@soxofaan
Copy link
Member Author

I'm not against unifying the parameters if it's feasible to detect whether something is a process or a level...

I think it can simply be something like: lower case ^\w+$ -> process id, upper case L\d+ -> profile level. And there is then even room left for additional possibilities (e.g. read from JSON file)

With regards to "not specified: cover all processes" please also note that experimental processes are not tested by default.

of course the "experimental" toggle can be kept orthogonal to that

@soxofaan soxofaan self-assigned this Jan 19, 2024
@m-mohr
Copy link
Member

m-mohr commented Jan 19, 2024

The level regexp might be misleading, would also match algo_for_L4_data. And just to make it clear, L3-Clim for example is also a valid level. Maybe use something like:
^L\d+ or ^L\d+(-.+)?$

For the processes I'm not 100% sure whether the OpenAPI patterns would lead to /^\w+$/ or /^\w+$/i actually... hmm.

@soxofaan
Copy link
Member Author

ok, regardless of unifying --processes and --process-levels, which might need some additional thinking, I still think we can make them work together in an additive way instead of documenting them confusingly as mutually exclusive:

  • neither is given: select all processes
  • --processes=min,max: only select min and `max
  • --process-levels=L1: only select L1 processes
  • --process-levels=L1 --processes=min,max: take union of L1 processes and [min, max]

unlike I previous stated, taking the union (instead of intersection) is probably more intuitive or handy in practice

@m-mohr
Copy link
Member

m-mohr commented Jan 21, 2024

I don't see a use case right now for it. I could see a use case for intersection (level + x processes), but not really for x within level. Why would you specify the level in this case?

@soxofaan
Copy link
Member Author

I think we're talking about the same thing, maybe my example was confusing as min/max are already in L1.
Using load_stac (L3 in https://openeo.org/documentation/1.0/developers/profiles/processes.html) is probably a more practical example:

--process-levels=L2 --processes=load_stac : allows to test some L2 workflows, using load_stac as input process

I could see a use case for intersection (level + x processes)

I think you mean "union" here. As you said, the other option "x within level" (intersection) is practically less interesting.

@m-mohr
Copy link
Member

m-mohr commented Jan 22, 2024

Indeed, a mixup on my side, sorry. It was too late...

soxofaan added a commit that referenced this issue Jan 23, 2024
soxofaan added a commit that referenced this issue Jan 23, 2024
soxofaan added a commit that referenced this issue Jan 23, 2024
@soxofaan soxofaan linked a pull request Jan 24, 2024 that will close this issue
soxofaan added a commit that referenced this issue Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants