Yaml configuration input #19

MichaelBarnett · 2024-10-27T23:39:01Z

Background

There was a need for better control on parsing database locations, as well as a system to easily add additional parameterisation into the commec screen program, that was not limited to CLI, thus removing the need for clunky shell/bash scripts.
Commec now expects a mandatory default "commec-config.yaml" file as input, found in the repository.

Dependencies:

pyyaml is required now, and has been added to the environment.yml python dependencies.

New features

The location of the file can be specified as cli input to Commec Screen, however by default it will search for commec-config.yaml, which is also now part of the repository.
Commec Setup CLI will update the base path of config file when databases are installed.
Database location no longer a mandatory input, as it can be derived from config yaml, however yaml behaviour can be overridden by forcing a database location in CLI when called Commec Screen.
Yaml file will always override the blast/diamond protein search tool string - consider removing blast/diamond cli input for screen - kept to make this a non-breaking change for now.
By parsing custom yaml files when calling Commec Screen, a user can easily automate the testing of different databases.
Allows end user ease of use when installing commec dbs using setup CLI.

Breaking changes

nil, commec screen should still function identically using old calling methods, where the database directory is set via screen CLI.

…th formatting, which is also optional.

…red, with default values of commec-dbs, to be grabbed from here, rather than updating the file paths after search handler creation.

…he databases to the correct filenames now.

…added an override such that that directory is used instead of default when specified to ensure a non-breaking update.

…t might make sense.

… now.

alexanian

Still need to run some more tests, but here's a few comments!

alexanian · 2024-11-29T01:52:08Z

commec/setup.py

@@ -617,6 +619,9 @@ def do_setup(self):

            os.remove(filename_zipped)

+        # We update the default path of the configuration file to point to the newly installed databases.


Two questions here:

Do we want to allow users to supply a configuration filepath (vs using the default one installed with the package)?

Should we move the name of the file (also referenced in ScreenConfig defaults) to a constants module so we can import it and it only lives in one place?

Resolved as:

No, it is nice to overwrite this (though there may be a touch of confusion when users update the package and overwrite the default; hopefully resolvable, though).

Yes; proposed in 8a3ac8d

alexanian · 2024-11-29T01:59:20Z

commec-config.yaml

@@ -0,0 +1,20 @@
+base_paths:


High-level suggestion: could we put all the defaults in here (vs. tracking some here and some in ScreenConfig)?

Could turn into something like:

base_paths: default: commec-dbs/ databases: benign: cm: path: '{default}benign_db/benign.cm' fasta: path: '{default}benign_db/benign.fasta' hmm: path: '{default}benign_db/benign.hmm' biorisk_hmm: path: '{default}biorisk_db/biorisk.hmm' regulated_nt: path: '{default}nt_blast/nt' regulated_protein: blast: path: '{default}nr_blast/nr' diamond: path: '{default}nr_dmnd/nr.dmnd' screen_logic: in_fast_mode: false skip_nt_search: false protein_search_tool: blastx parallelisation: threads: 1 output_handling: force: false resume: false

We could then throw an error if we can't find the default config.yaml and are missing overrides from the CLI or a user-supplied config? We already raise a FileNotFoundError if the default yaml can't be found. This wouldn't require users to change their CLI calls (we're just moving around where the defaults are stored) I think.

This is 100% the end-game purpose of the yaml, I just didn't add all the other commands from screen yet , as just wanted to get the core of it (yaml system and db directories) up and running.
Could certainly update the parsing to include all relevant cli args in this PR though.

We decided that we want to do this eventually, but this should be backlogged, and for now we just want to remove the protein_search_tool piece (since then the config file is more cleanly just about specifying databases, to a level of specificity that was not possible before), proposed change in: 059ad43

alexanian · 2024-11-29T07:02:55Z

commec/config/io_parameters.py

+                "but was invalid as a yaml file:\n",
+                e,
+            )
+            return {}


I think this return value is unused? I'm a bit confused about this vs line 172

Looking at it more, I think we should decide if the desired behavior here is "raise an exception" (would match with raising an exception when the file is not found) or "no YAML overrides" (probably not viable now that we made the databases argument optional)?

Now raises an exception as of 8409257

commec/config/screen_tools.py

(since I found it a bit confusing that, when not providing `--databases`, I got a "not found" error for a place I had never told the code to look)

alexanian

Tested out; clearly going to be very useful!

I'm hoping @MichaelBarnett you can take a quick look over before squash + merge, since I made a few of the changes we talked about this afternoon (e.g. removing the protein_search_tool key in the default config).

alexanian · 2024-12-03T05:55:41Z

commec/setup.py

@@ -617,6 +619,9 @@ def do_setup(self):

            os.remove(filename_zipped)

+        # We update the default path of the configuration file to point to the newly installed databases.


Resolved as:

No, it is nice to overwrite this (though there may be a touch of confusion when users update the package and overwrite the default; hopefully resolvable, though).

Yes; proposed in 8a3ac8d

alexanian · 2024-12-03T05:57:02Z

commec/config/io_parameters.py

+                "but was invalid as a yaml file:\n",
+                e,
+            )
+            return {}


Now raises an exception as of 8409257

alexanian · 2024-12-03T05:58:14Z

commec-config.yaml

@@ -0,0 +1,20 @@
+base_paths:


We decided that we want to do this eventually, but this should be backlogged, and for now we just want to remove the protein_search_tool piece (since then the config file is more cleanly just about specifying databases, to a level of specificity that was not possible before), proposed change in: 059ad43

MichaelBarnett · 2024-12-04T21:06:41Z

Ran some tests, and is working as intended, nicely done on the updates here, looking forward to working with adding in the rest of the settings soon too!

Michael Barnett added 13 commits October 21, 2024 19:44

Added basic yaml input and parsing, with custom argument, and base pa…

7009ae0

…th formatting, which is also optional.

Bugfix yaml file error checking, and base path parsing.

25210c7

Switched parsing of yaml, to be in io_parameters, now acts as a requi…

072f134

…red, with default values of commec-dbs, to be grabbed from here, rather than updating the file paths after search handler creation.

Synced the outputs of the yaml to the screen_tools, correctly sends t…

a15ab9f

…he databases to the correct filenames now.

Removed required nature of adding a directory for databases, however …

a697fc2

…added an override such that that directory is used instead of default when specified to ensure a non-breaking update.

Tidying default commec-config.yaml

144e10d

Merge remote-tracking branch 'origin/main' into yaml-input

4b9d295

Rearranged third party imports, updated yml config with a default tha…

e3f2818

…t might make sense.

Updated the default config base path when using cli setup.

5508e15

Reset defaults in config file, removed some erroneous print messages.

dfed3f2

Reset defaults in config file, removed some erroneous print messages.

36ec715

minor format changes

a689e5b

Added pyyaml to the environment, as it is needed during configuration…

ac5262a

… now.

MichaelBarnett requested a review from alexanian October 27, 2024 23:39

alexanian added 2 commits November 28, 2024 17:17

Merge branch 'main' into yaml-input

c60ab39

variable name updates to amke easier to follow

d47bb2e

alexanian reviewed Nov 29, 2024

View reviewed changes

alexanian added 9 commits December 2, 2024 14:21

probably should defauly to UK spelling

2d40c2b

Remove "protein_search_tool" key from YAML for now

059ad43

Remove print() statements

5899581

Add a few more error checks to YAML parsing

8409257

Update usage/logs in screen

b74c2e1

Can't actually log if there is no logger yet

91cfc1c

Add info about how to set screening directory to not found error

a932c44

(since I found it a bit confusing that, when not providing `--databases`, I got a "not found" error for a place I had never told the code to look)

Add even more info about how to use YAML

9a4c64b

Add config.constants (hope to later expand)

8a3ac8d

alexanian approved these changes Dec 3, 2024

View reviewed changes

MichaelBarnett merged commit bb75e5b into main Dec 4, 2024

MichaelBarnett deleted the yaml-input branch December 4, 2024 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yaml configuration input #19

Yaml configuration input #19

MichaelBarnett commented Oct 27, 2024

alexanian left a comment

alexanian Nov 29, 2024

alexanian Dec 3, 2024

alexanian Nov 29, 2024 •

edited

Loading

MichaelBarnett Nov 30, 2024

alexanian Dec 3, 2024

alexanian Nov 29, 2024

alexanian Dec 2, 2024

alexanian Dec 3, 2024

alexanian left a comment

alexanian Dec 3, 2024

alexanian Dec 3, 2024

alexanian Dec 3, 2024

MichaelBarnett commented Dec 4, 2024

		@@ -617,6 +619,9 @@ def do_setup(self):

		os.remove(filename_zipped)

		# We update the default path of the configuration file to point to the newly installed databases.

Yaml configuration input #19

Yaml configuration input #19

Conversation

MichaelBarnett commented Oct 27, 2024

Background

Dependencies:

New features

Breaking changes

alexanian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanian Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelBarnett commented Dec 4, 2024

alexanian Nov 29, 2024 •

edited

Loading