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

BD rhapsody sequence analysis #96

Merged
merged 51 commits into from
Sep 17, 2024
Merged

BD rhapsody sequence analysis #96

merged 51 commits into from
Sep 17, 2024

Conversation

rcannood
Copy link
Contributor

@rcannood rcannood commented Jul 27, 2024

Description

Issue ticket number

Closes #xxxx

Checklist before requesting a review

  • I have performed a self-review of my code

  • Conforms to the Contributing guidelines

  • Proposed changes are described in the CHANGELOG.md

  • I have tested my code with viash ns test --parallel -q <name or namespace>

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • New functionality
    • Major changes
    • Minor changes
    • Documentation
    • Bug fixes

@rcannood rcannood requested a review from DriesSchaumont August 8, 2024 06:19
@rcannood rcannood requested a review from dorien-er August 8, 2024 06:38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would propose to also create an unpublished component for this, so that it it easier to run and manage the dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙅 No thank you! It will never be used as part of a pipeline or a standalone component. The script uses just base tidyverse. And oh, one dynutils statement which probably has an equivalent function in the tidyverse.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but in that case I would add a short README with perhaps the following:

  • What the script does
  • The dependencies required
  • How to run the script

Just in case somebody wants to work on this in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

src/bd_rhapsody/bd_rhapsody_sequence_analysis/script.py Outdated Show resolved Hide resolved
par_value = par[arg["clean_name"]]
if par_value and arg["type"] == "file" and arg["direction"] == "output":
# example template: '[sample_name]_(assay)_cell_type_experimental.csv'
template = (arg.get("info") or {}).get("template")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
template = (arg.get("info") or {}).get("template")
template = (arg.get("info", {})).get("template")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a pitfall but this doesn't work! Because info might be null which causes arg.get("info", {}) to return None.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so if I understand correctly you get a TypeError when calling .get("template"). Could you add a small comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added note. Sorry for the delay!

src/bd_rhapsody/bd_rhapsody_sequence_analysis/script.py Outdated Show resolved Hide resolved
src/bd_rhapsody/bd_rhapsody_sequence_analysis/script.py Outdated Show resolved Hide resolved
src/bd_rhapsody/bd_rhapsody_sequence_analysis/script.py Outdated Show resolved Hide resolved

# Run command
print("> " + ' '.join(cmd), flush=True)
_ = subprocess.check_call(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the cwl pipeline create a log file by itself? Otherwise I would prefer to use check_output and write it ourself from stdout and stderr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean out = subprocess.check_output(cmd_pars).decode("utf-8") and then printing the stdout?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, printing it, or writing it to an output file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to view the output as it comes out of the process ^^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Printing it is also fine. But right now, neither one of those is happening (because check_call is used). https://docs.python.org/3/library/subprocess.html#subprocess.check_call

Code needing to capture stdout or stderr should use [run()](https://docs.python.org/3/library/subprocess.html#subprocess.run) instead:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. subprocess.check_call does exactly what I want it to do.

>>> out = subprocess.check_call(["echo", "hi"])
hi

For sure, subprocess.run seems to do exactly the same:

>>> out = subprocess.run(["echo", "hi"], check=True)
hi

I guess I don't understand what the difference between the two variants is and why you'd like me to use subprocess.run over subprocess.check_call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that the pipeline already returns its own log in the output, so this has become a non-issue 👍

src/bd_rhapsody/bd_rhapsody_sequence_analysis/script.py Outdated Show resolved Hide resolved
src/bd_rhapsody/bd_rhapsody_sequence_analysis/script.py Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but in that case I would add a short README with perhaps the following:

  • What the script does
  • The dependencies required
  • How to run the script

Just in case somebody wants to work on this in the future


# Run command
print("> " + ' '.join(cmd), flush=True)
_ = subprocess.check_call(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Printing it is also fine. But right now, neither one of those is happening (because check_call is used). https://docs.python.org/3/library/subprocess.html#subprocess.check_call

Code needing to capture stdout or stderr should use [run()](https://docs.python.org/3/library/subprocess.html#subprocess.run) instead:

@rcannood rcannood merged commit 7f8bcc2 into main Sep 17, 2024
3 checks passed
@rcannood rcannood deleted the bd_rhapsody_sequence_analysis branch September 17, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants