bioforensics / yeat

YEAT: Your Everyday Assembly Tool
Other
1 stars 0 forks source link

Adding multi-assembly config support #16

Closed danejo3 closed 1 year ago

danejo3 commented 1 year ago

The purpose of this PR is to allow users to create their own assembly config file.

An example of the config file can be seen by executing yeat --init, which will pull up:

[
    {
        "assembler": "spades",
        "extra_args": ""
    },
    {
        "assembler": "megahit",
        "extra_args": ""
    }
]
danejo3 commented 1 year ago

Tests are passing. Coverage is at 99%. Running full test suite is a bit slow because two tests are running spades, megabit, and unicycler. Will work on #8 to resolve this issue.

danejo3 commented 1 year ago

This PR is ready for review!

-Created a config.py class to parse through the user's config file. -Added a command to display a template config file -No changes to the main snakefile made; will be updating it in the next PR to resolve several issue threads like #17 and #18 .

danejo3 commented 1 year ago

Thanks for the comments and suggestions in #19 ! Changes and suggestions were incorporated. All tests passing!

standage commented 1 year ago

It looks like you didn't merge #19 into this branch. Did you copy and paste the suggested changes? Or did you adapt them? It's hard to tell.

standage commented 1 year ago

I'll also note that it doesn't appear the extra arguments are actually passed to the Snakefile and used when running the assembly tools.

Has this been resolved? Could you point me to where the extra arguments are used when running the various assemblers?

danejo3 commented 1 year ago

It looks like you didn't merge #19 into this branch. Did you copy and paste the suggested changes? Or did you adapt them? It's hard to tell.

I did not merge #19 original, but copied pasted. After you mentioned the word merging, I realized that I could have done just that! 😅 Did I merge the suggest/config branch correctly? Hopefully the git commands that I ran were correct.

danejo3 commented 1 year ago

I'll also note that it doesn't appear the extra arguments are actually passed to the Snakefile and used when running the assembly tools.

Has this been resolved? Could you point me to where the extra arguments are used when running the various assemblers?

In the PR's description, I have mentioned that the extra_args value in the config file has no functionality. I was planning on opening up a new MR to resolve the non-functional feature. I did this because I wanted to keep this PR relatively small. However, since the extra_args data in the config file has been added, I guess the idea that extra_args is functional should be added into this branch as well. (I probably should have made a different branch with the extra_args and its' implemented functionality.) Anyways, I am currently adapting the snakelike file to execute the extra args for the various assembly algorithms. I'll have it in the next push.

standage commented 1 year ago

Did I merge the suggest/config branch correctly?

The idea of opening PR #19, like all PRs, was to have you review it, respond if needed, and merge it. The only difference, as mentioned, is that #19 updated this branch instead of main. It sounds like you took a different route and merged it on the command line? In any case, whatever you did seems to have integrated my suggestions and closed #19.

standage commented 1 year ago

I was planning on opening up a new MR to resolve the non-functional feature.

Ah I see, I misunderstood that.

I speak often about managing the scope of each PR so that effective review can be done with a feasible amount of time and effort. Was the idea here to split the changes up so that they're more manageable? If so, I'd recommend that you go ahead and include that feature: it's within scope of this branch; this branch isn't terribly large; only marginal additional changes to the code are required; and merging a partially implemented feature is something we typically want to avoid unless it really can't be helped.

If that wasn't the idea, please clarify.

danejo3 commented 1 year ago

Did I merge the suggest/config branch correctly?

The idea of opening PR #19, like all PRs, was to have you review it, respond if needed, and merge it. The only difference, as mentioned, is that #19 updated this branch instead of main. It sounds like you took a different route and merged it on the command line? In any case, whatever you did seems to have integrated my suggestions and closed #19.

Oh, I see. Yes, I was under the impression that I needed to merge it back into my branch.

danejo3 commented 1 year ago

Was the idea here to split the changes up so that they're more manageable?

Yeah, I'm trying to break down my tasks to more manageable chucks for people to follow.

If so, I'd recommend that you go ahead and include that feature: it's within scope of this branch; this branch isn't terribly large; only marginal additional changes to the code are required; and merging a partially implemented feature is something we typically want to avoid unless it really can't be helped.

Sounds good. Updating the code as we speak!

standage commented 1 year ago

I was under the impression that I needed to merge it back into my branch.

The big green [Merge] button on the PR is provided conveniently for that purpose. :smiley:

This is a less common use case, but it can be helpful when proposing changes to an existing branch/PR. In the future, if you approve of the suggested changes, you can just approve and merge the PR and it will update your existing branch/PR. No command line drudgery required.

danejo3 commented 1 year ago

I don't feel very confident with the following changes that I've made, but it does work! I feel like there is a way better solution to this, but I am unsure what I can/could do. I am up for any suggestions!

danejo3 commented 1 year ago

Thanks for all of your suggestions!