search

OpenMS / streamlit-template

5 stars 10 forks source link

Add basic handling for flags #59

Open poshul opened 6 months ago

poshul commented 6 months ago

PR Type

enhancement

Description

Changes walkthrough 📝

Relevant files
Enhancement
CommandExecutor.py
Enhance Flag Handling in Command Construction                       
src/workflow/CommandExecutor.py
  • Enhanced command construction logic to handle flags more
    appropriately.
  • Flags are now checked for their boolean value, and only the flag name
    is added if the value is true.
  • Skips appending the value for boolean flags to ensure correct command
    formation.
    		•  +8/-2     

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-pro[bot] commented 6 months ago

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Description updated to latest commit (https://github.com/OpenMS/streamlit-template/commit/34503ac39bf44d6c050d2f47bac92b89c3337983)

    codiumai-pr-agent-pro[bot] commented 6 months ago

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5] 2, because the changes are localized to a specific functionality in the code, and the logic is straightforward. The PR modifies the command line flag handling in a Python script, which is a common task and should be relatively easy to review for an experienced developer.
    🧪 Relevant tests No
    ⚡ Possible issues Possible Bug: The condition `if v != False:` might not behave as expected if `v` is `None` or an empty string, which are also falsy values in Python. This could lead to unintended flags being added to the command.
    Code Clarity: The comment `# add the value if we aren't a flag` might be misleading because it's placed before a block that handles multiline strings, not specifically non-flag values.
    🔒 Security concerns No
    codiumai-pr-agent-pro[bot] commented 6 months ago

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Best practice
    Use identity comparison for boolean checks. ___ **Replace the check if v != False with if v is not False to ensure that the comparison is
    done using identity rather than equality. This is more Pythonic and avoids potential
    issues with different types that may evaluate to false.** [src/workflow/CommandExecutor.py [170]](https://github.com/OpenMS/streamlit-template/pull/59/files#diff-8415106457fd9d1400b9074c525816f067a131350c54c9ec9927efbc2f32162aR170-R170) ```diff -if v != False: +if v is not False: ```
    Enhancement
    Add a check for None values to prevent errors. ___ **Consider adding a check for None values before appending to the command list to avoid
    potential errors when None is inadvertently passed as a value.** [src/workflow/CommandExecutor.py [179]](https://github.com/OpenMS/streamlit-template/pull/59/files#diff-8415106457fd9d1400b9074c525816f067a131350c54c9ec9927efbc2f32162aR179-R179) ```diff -command += [str(v)] +if v is not None: + command += [str(v)] ```
    Add error handling for file removal operations. ___ **Ensure that the temporary file is safely removed by using a try-except block to handle
    potential exceptions during the unlink operation.** [src/workflow/CommandExecutor.py [274]](https://github.com/OpenMS/streamlit-template/pull/59/files#diff-8415106457fd9d1400b9074c525816f067a131350c54c9ec9927efbc2f32162aR274-R274) ```diff -tmp_params_file.unlink() +try: + tmp_params_file.unlink() +except Exception as e: + print(f"Failed to unlink file: {e}") ```
    Maintainability
    Clarify the condition for handling boolean flags. ___ **Use a more explicit condition check for boolean flags by checking if isinstance(v, bool)
    and v: instead of just elif isinstance(v, bool):. This ensures that the logic is clear and
    only true boolean flags are considered.** [src/workflow/CommandExecutor.py [176]](https://github.com/OpenMS/streamlit-template/pull/59/files#diff-8415106457fd9d1400b9074c525816f067a131350c54c9ec9927efbc2f32162aR176-R176) ```diff -elif isinstance(v, bool): +elif isinstance(v, bool) and v: ```
    Refactor command construction into a separate method. ___ **Refactor the command construction logic into a separate method to improve readability and
    maintainability of the run_topp method.** [src/workflow/CommandExecutor.py [167-179]](https://github.com/OpenMS/streamlit-template/pull/59/files#diff-8415106457fd9d1400b9074c525816f067a131350c54c9ec9927efbc2f32162aR167-R179) ```diff -if tool in params.keys(): - for k, v in params[tool].items(): +def construct_command(params): + command = [] + for k, v in params.items(): if v is not False: - command += [f"-{k}"] + command.append(f"-{k}") if isinstance(v, str) and "\n" in v: - command += v.split("\n") + command.extend(v.split("\n")) elif isinstance(v, bool) and v: continue else: - command += [str(v)] + command.append(str(v)) + return command ```
    axelwalter commented 6 months ago

    Running this with the example workflow results in errors for non-flag boolean values, which require "true" or "false" passed as string.

    Invalid string parameter value '' for parameter 'enable_RT_filtering' given! Valid values are: 'false,true'. Updating failed!

    axelwalter commented 6 months ago

    Setting the value depending on bool or string type might solve it? https://github.com/OpenMS/streamlit-template/blob/bbbd104183a58aa8b2c975a6e9cf0fcfdbd0f4f2/src/workflow/StreamlitUI.py#L420

    poshul commented 6 months ago

    Running this with the example workflow results in errors for non-flag boolean values, which require "true" or "false" passed as string.

    Invalid string parameter value '' for parameter 'enable_RT_filtering' given! Valid values are: 'false,true'. Updating failed!

    Hmm, thats a problem. Underlying this is gonna be the fact that since ParamValue doesn't have a type for Bool (https://github.com/OpenMS/OpenMS/blob/develop/src/openms/include/OpenMS/DATASTRUCTURES/ParamValue.h#L37-L46) and the "type" field from the ini file doesn't get stored, that I don't think we can tell the difference between a flag and a string option that just has true or false as its options from the option itself.

    Maybe we can check the allowable values as well to distinguish?

    cbielow commented 6 days ago

    Running this with the example workflow results in errors for non-flag boolean values, which require "true" or "false" passed as string. Invalid string parameter value '' for parameter 'enable_RT_filtering' given! Valid values are: 'false,true'. Updating failed!

    Hmm, thats a problem. Underlying this is gonna be the fact that since ParamValue doesn't have a type for Bool (https://github.com/OpenMS/OpenMS/blob/develop/src/openms/include/OpenMS/DATASTRUCTURES/ParamValue.h#L37-L46) and the "type" field from the ini file doesn't get stored, that I don't think we can tell the difference between a flag and a string option that just has true or false as its options from the option itself.

    Maybe we can check the allowable values as well to distinguish?

    use ParamValue::toBool()