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

CLI argument handler restructuring #16

Merged
merged 17 commits into from
Aug 9, 2022
Merged

Conversation

axiomcura
Copy link
Member

Motivation

The motivation of this PR is in regards to how cytopipe handles user input arguments from the terminal.

The issue I was facing when working with the previous version of the args module was how fragile it was when introducing different argument patterns.

For example, if I were to develop a new mode that required some user parameters, the args_checker() function required to be completely re-written. This has to not to only accommodate new features being implemented but also the previously designed features.

approach

To approach this problem, I decided to create a class object known as CliControlPanel. The class keeps track of all supported parameters, workflows and argument states.

A unique feature of this class is usage of argument_states. These states are simply boolean data type that tell cytopipe which parameters were selected. This provides easy control flow of different parameters and what operations to execute during run time.

In addition, this allows implementing more features much simpler, since we are only dealing with boolean operators!

The majority of the CliControlPanel implemenation is done within this commit: bcbf011

@axiomcura axiomcura requested a review from gwaybio August 8, 2022 21:28
Copy link
Member

@gwaybio gwaybio left a comment

Choose a reason for hiding this comment

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

LGTM, a couple minor comments. Merge at will!

cytopipe/cli/args.py Outdated Show resolved Hide resolved
cytopipe/cli/cmd.py Show resolved Hide resolved
# create data folder
# raises error if data directory exists (prevents overwriting)
data_dir_obj = Path("data")
data_dir_obj.mkdir(exist_ok=False)
Copy link
Member

Choose a reason for hiding this comment

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

What if force == True?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not know if I explained to you about snakemake's ability to track the progress of the workflow and sets up save points for each successful rule. This is beautifully illustrated from this paper on figure 1 far right figure.

if --force where to be false, this indicates that when you re-run cytopipe it will start from where it failed. If --force where to be True, then executing cytopipe run again will cause the workflow to start from the beginning and recreate the files again.

Hopefully that clears any confusion!

Copy link
Member

Choose a reason for hiding this comment

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

ooohh, so if the directory is created during cytopipe, if the cytopipe run fails, the directory will be deleted?

(i can't see fig 1 b/c paywall)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily. Remember, the data directory is important to maintain. That's how snakemake knows where to put the outputs files. What occurs is that snakemake knows that there are files within the data folder and when --force flag is True, you are telling snakemake (via cytopipe) "Hey, I know there are files there in there, but I want you to start from the beginning and overwrite them"

The data directory does not get deleted. The contents within the data folder gets overwritten.

Hopefully this makes sense!

P.S: I have sent the paper I was referring via Discord!

cytopipe/cli/cmd.py Show resolved Hide resolved
cytopipe/cli/exec/workflow_exec.py Outdated Show resolved Hide resolved
cytopipe/common/errors.py Outdated Show resolved Hide resolved
@axiomcura
Copy link
Member Author

Hey! @gwaybio

I have answered one of your comments here. Hopefully my responses provide some clarity of the implementation.

I ended up removing the use_conda_env parameter due to the consequence of providing the user too much flexibility (E.g using their own environments) and may greatly affect reproducibility. commit: f80f6fc

I will be merging this now!

@axiomcura axiomcura merged commit 2dc5960 into WayScience:main Aug 9, 2022
@axiomcura axiomcura deleted the cli_revamp branch September 2, 2022 18:30
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