End repair

[jmtsuji]

Addresses #58 so that repair.py is installed as part of rotary and is integrated into the snakemake workflow. Also includes usage of TIGRFAM HMMs.

@LeeBergstrand would you mind taking a look before this PR is merged? (Thanks so much for your support for the install of repair.py!!)

[LeeBergstrand]

@jmtsuji I added e4dceff to add the dependencies to the other requirements files. So, three files require dependency updates.

• environment.yaml - used for creating conda enviroments.
• meta.yaml - used by conda build when building conda packges.
• pyproject.toml - used by pip install when installing via ```pip``. It can only install pure Python packages offPypii (so no bioinformatics tools).

You could technically remove environment.yaml once the package is uploaded to Anaconda as conda build would specify these dependencies. You can programmatically get the dependencies out of the environment.yaml and have them templated during the build process (see https://stackoverflow.com/questions/60125363/conda-meta-yaml-requirements-run-section-dependencies-from-file)

The conda build process works like this:

1. Conda build takes the meta.yaml file and uses it to start building the Conda package.
2. Conda build installs the dependencies listed in the meta.yaml file.
3. Conda build calls pip install . to install the rotary python package.
4. pip install uses the the dependencies in pyproject.toml and attempts to install them. However, these were already installed by ```conda build,` so it skips this step.
5. pip install uses the the scripts list in pyproject.toml to install the CLI scripts.
6. All the resulting dependencies and files are baked by conda build into a conda bundle specific to the operating system the package was built on.
7. You then run conda convert to convert the package you built for your OS to versions that work on other operating systems (this is why, on anaconda.org, you see multiple package builds for different OSes). If the conversion fails, you must run conda build on a different OS. For example, if the Windows conversion build fails, you must run conda build on a Windows computer and upload it separately.
8. You then use the anaconda CLI tool to upload these packages to anaconda.org.

The big projects run these steps using GitHub Actions (https://docs.github.com/en/actions) to do the build and upload steps.

[LeeBergstrand]

Great job putting this together! Looks like it's coming together splendidly. I left a lot of comments, so keep going.

Okay, right now, repair.py has this design pattern where you pass down the dependency_dict to all the child functions that call subprocesses. This method could be more straightforward to a degree.

Right now, your code is like the following:

samtools_sort_args = [dependency_dict['samtools'], 'sort', '-@', str(threads), '-m', f'{threads_mem_mb}M']
subprocess.run(samtools_sort_args, check=True, input=samtools_view.stdout, stdout=bam_handle, stderr=logfile_handle)

I want to move your code to using shell=true. This runs the sub-command in a shell and thus can call the underlying programs like samtools without a path. There are security issues with shell=true, but they don't apply to us because we already run this tool from CLI.

So you can just run:

samtools_sort_args = ['samtools', 'sort', '-@', str(threads), '-m', f'{threads_mem_mb}M']

[LeeBergstrand]

What are your thoughts on removing the CLI dependency check and the dependancy_dict from the code? If the dependency isn't there, the code would fail, and it would dump that the shell can't find it. Installing through conda should guarantee that the dependencies are installed 98% of the time.

[LeeBergstrand]

The following code should be moved to a function as it is repeated multiple times.

 if append_log is True:
        write_mode = 'a'
    elif append_log is False:
        write_mode = 'w'
    else:
        raise ValueError

I would also add an ValueError(fstring) / error.log to the explain why the value error is being raised.

[jmtsuji]

@jmtsuji I added e4dceff to add the dependencies to the other requirements files. So, three files require dependency updates.

@LeeBergstrand Thanks for the explanation and for updating the other requirement files! I'll keep this in mind when updating dependencies in the future. Your overview of how the pip and conda installers interact is very helpful.

[jmtsuji]

The following code should be moved to a function as it is repeated multiple times...

Moved to a function

[jmtsuji]

Okay, right now, repair.py has this design pattern where you pass down the dependency_dict to all the child functions that call subprocesses. This method could be more straightforward to a degree...
I want to move your code to using shell=true. This runs the sub-command in a shell and thus can call the underlying programs like samtools without a path. There are security issues with shell=true, but they don't apply to us because we already run this tool from CLI.

@LeeBergstrand Actually, based on my previous testing, I am pretty sure that I can already call the underlying programs like samtools without a path, even if shell = False. However, I decided to explicitly set the path to the programs to avoid any weird PATH conflict issues that might happen. Do you think that such path conflicts are basically impossible? If so, I can simplify the code.

[jmtsuji]

What are your thoughts on removing the CLI dependency check and the dependancy_dict from the code? If the dependency isn't there, the code would fail, and it would dump that the shell can't find it. Installing through conda should guarantee that the dependencies are installed 98% of the time.

@LeeBergstrand I like the CLI dependency check because it tells the user upfront that a dependency is missing. Otherwise, they might have to run 50% of the pipeline before they run into the missing dependency issue. In future, I am thinking of using the dependency check function to also grab the version of the dependency tool to add to the log. So I would be happy to leave the function in the code... what are your thoughts?

[jmtsuji]

Great job putting this together! Looks like it's coming together splendidly. I left a lot of comments, so keep going.

@LeeBergstrand Thanks! Appreciate the comments. I left several responses and some unresolved threads, so please have a look and let me know your thoughts... we can move forward toward merging this PR based on your feedback.

[LeeBergstrand]

@LeeBergstrand Actually, based on my previous testing, I am pretty sure that I can already call the underlying programs like samtools without a path, even if shell = False. However, I decided to explicitly set the path to the programs to avoid any weird PATH conflict issues that might happen. Do you think that such path conflicts are basically impossible? If so, I can simplify the code.

Given the installation, these pathing issues are generally unlikely. Because check_dependency() uses shutil.which(dependency_name), you will likely pass any pathing issues onto the Python code. I believe the shutil.which would be affected by shell variables, symlinks, etc., including incorrect ones. In Unix, executables are files with some file system flags changed, and globally accessible executables are those added to the $PATH. The default $PATH for your user should include /home/YOUR-USER-NAME/bin (i.e. ~/bin). Most executables are symlinked to ~/bin, which lets them show up in your shell when you call them.

Given the above I think we can simplify the code.

[LeeBergstrand]

What are your thoughts on removing the CLI dependency check and the dependancy_dict from the code? If the dependency isn't there, the code would fail, and it would dump that the shell can't find it. Installing through conda should guarantee that the dependencies are installed 98% of the time.

@LeeBergstrand I like the CLI dependency check because it tells the user upfront that a dependency is missing. Otherwise, they might have to run 50% of the pipeline before they run into the missing dependency issue. In future, I am thinking of using the dependency check function to also grab the version of the dependency tool to add to the log. So I would be happy to leave the function in the code... what are your thoughts?

If the end repair pipeline takes a while, I would keep the dependency check-in. I think logging the dependency versions would be helpful.

[jmtsuji]

Given the installation, these pathing issues are generally unlikely. Because check_dependency() uses shutil.which(dependency_name), you will likely pass any pathing issues onto the Python code. I believe the shutil.which would be affected by shell variables, symlinks, etc., including incorrect ones. In Unix, executables are files with some file system flags changed, and globally accessible executables are those added to the $PATH. The default $PATH for your user should include /home/YOUR-USER-NAME/bin (i.e. ~/bin). Most executables are symlinked to ~/bin, which lets them show up in your shell when you call them.
Given the above I think we can simplify the code.

OK, got it. I have simplified the code!

[jmtsuji]

If the end repair pipeline takes a while, I would keep the dependency check-in. I think logging the dependency versions would be helpful.

End repair might take several minutes (or longer if a large assembly is input), so let's leave dependency_check() in the code. I'll continue to aim to add version checks in the future.

[LeeBergstrand]

@jmtsuji We are pretty close to being able to merge this in—one last issue other than removing the dependency_dict,

https://github.com/jmtsuji/rotary/blob/0b984a5dd01d80f999d0f688c9aac3fb4c228ffe/rotary/repair.py#L463

Is this the correct function argument mapping? In the other runs commands, you mapped stderr to the log file. These arguments are mapped correctly?

[jmtsuji]

Is this the correct function argument mapping? In the other runs commands, you mapped stderr to the log file. These arguments are mapped correctly?

@LeeBergstrand Yes, this is correct. This particular tool writes the log info to STDOUT, so I had to direct STDOUT to the logfile. I wanted to retain STDERR info as well in case anything got printed there. (By the way, this is the tool that I plan to replace with in-house code).

[LeeBergstrand]

Is this the correct function argument mapping? In the other runs commands, you mapped stderr to the log file. These arguments are mapped correctly?

@LeeBergstrand Yes, this is correct. This particular tool writes the log info to STDOUT, so I had to direct STDOUT to the logfile. I wanted to retain STDERR info as well in case anything got printed there. (By the way, this is the tool that I plan to replace with in-house code).

Great I wanted to confirm.

[jmtsuji]

@LeeBergstrand Dependency dict and logger changes are addressed. End-to-end tests are passing. OK to merge?

[LeeBergstrand]

@jmtsuji Great. Feel free to merge.