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

[BUG] Default value for Parameter not used in execution #193

Open
jugdemon opened this issue Nov 28, 2024 · 7 comments
Open

[BUG] Default value for Parameter not used in execution #193

jugdemon opened this issue Nov 28, 2024 · 7 comments
Assignees
Labels
bug Something isn't working release0.5.0

Comments

@jugdemon
Copy link
Member

Environment

  • ODTP/ODTP Component version: odtp-eqasim
  • Operating System: Ubuntu 24.04.1 LTS
  • Browser: (if applicable)

Bug description

The default value for a parameter is not used.

Summary:

We setup the eqasim pipeline. We don't provide parameters because the default is set to the scenario we want to test. The execution fails for a parameter. First the scenario was CH instead of Corsica and second hts was not set after we forced scenario to Corsica. Both scenario and hts have defaults that should load Corsica with the entd.

Steps to reproduce the bug:

  1. Create eqasim pipeline
  2. Don't add parameters
  3. Run

Expected behavior

The default parameters are loaded and the full pipeline is executed.

Actual behavior

The execution fails because parameters are not set.

Severity and impact

  • Severity: Critical
  • Impact: A simple demo cannot be shown because all parameters need to be meticously set defeating the purpose of defaults.

Supporting information

  • Upon request.
@jugdemon jugdemon added the bug Something isn't working label Nov 28, 2024
@sabinem sabinem self-assigned this Dec 2, 2024
@sabinem
Copy link
Contributor

sabinem commented Dec 3, 2024

@jugdemon Currently we are not reading the odtp.yml file in odtp. But it is possible to get the default parameters from there and to store them on the component version. So what you want here is clear and makes sense to me.

@sabinem
Copy link
Contributor

sabinem commented Dec 4, 2024

@jugdemon One problem I see here is that the odtp.yml files have not been checked in previous versions of the components to make sure that they contain valid yaml.

  • So for the future we can implement this check for the components
  • Some existing versions of components should then be deprecated as they don't have valid yaml files.
    Would that be okay for you?

@jugdemon
Copy link
Member Author

jugdemon commented Dec 4, 2024

I believe that is absolutely necessary. I think loading a component without a valid yml should cause s warning to appear in odtp indicating limited functionality. We can also indicates that in the future these component with no longer work.

Maybe we can have a developer mode in odtp that can be enabled to work with incomplete ymls then? It would be nice to complete them in the UI but I guess that would require the right to write to the components repo - so that is probably out of scope for now. But odtp setting with a dev mode to test a component with an incomplete yml could ne interesting.

I guess those odtp setting could also be where you can set the link to the zoo you want to use. Should we make a new issue about settings as to keep this clean?

@sabinem
Copy link
Contributor

sabinem commented Dec 5, 2024

@jugdemon

Maybe we can have a developer mode in odtp that can be enabled to work with incomplete ymls then?

I think the development mode is the cli not the gui. In the GUI I would like to keep it simple.

I would then skip the step in the execution run, where parameters can be uploaded from file and just add the parameter overwrite step: so for steps where default parameters are missing: all parameters would have to be added manually. But anyway these component versions are marked in the UI and should not be used any more.

When you add a new component version a parsable odtp.yml will be required and you will not be able to add it unless you have corrected the odtp.yml.

Yes for a link to the zoo, that would be another ticket.

@sabinem
Copy link
Contributor

sabinem commented Dec 18, 2024

@jugdemon I need to know what flexibility to provide in regards to adding ports and parameters for a step:

  1. Do I need to only offer ports and parameters that are declared in odtp.yml and they can be taken as is or overwritten?
  2. When I offer parameter uploads from file: the number of parameters can be flexible and I just replace all other parameters for the step. (should I only accept parameters that have been declared in odtp.yml or any parameters?
  3. I don't offer file picker for parameters any more, just upload from file, is that correct?

can you please describe exactly the flexibility that you need in that regard?

@jugdemon
Copy link
Member Author

@sabinem regarding

  1. I believe all parameters should be declared in odtp.yml (as it is the authorative config). If it is not there, the parameter/ports should not be supported. We don't want secret hidden parameters which would go against the spirit of open science or security concerns. The parameters in the odtp.yml are the defaults so as soon as users provide parameters they should take precedence.
  2. I think we should only accept existing parameters. Maybe post a warning that an undeclared parameter was supplied and name the parameter and value.
  3. I think it would be nice to have both - some params may be stored on the server whereas other parameters are user-supplied. I outlined the reason for server params in the other ticket but I understand that development make be limited to only one options. In terms of usability, we will reuqire the parameter upload.

@jugdemon
Copy link
Member Author

@sabinem With regards to CLI-based development: As we want to offer ODTP as a service on our server where people cannot access GUI, at some point testing/developing on ODTP would be nice. I am not talking about creating components but about how to string them together and check that all parameters have been configured correctly. When we tried to make the corisca example work we needed a few attempts and that was so clunky that I am afraid people might stop before properly using it. I think this is more of a long-term goal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release0.5.0
Projects
None yet
Development

No branches or pull requests

2 participants