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

Race conditions with config.yml #346

Closed
kdm9 opened this issue Apr 11, 2023 · 3 comments · Fixed by #376
Closed

Race conditions with config.yml #346

kdm9 opened this issue Apr 11, 2023 · 3 comments · Fixed by #376

Comments

@kdm9
Copy link
Contributor

kdm9 commented Apr 11, 2023

Thanks for AGAT, i've found it very useful. I've just updated to v1.0.0 to fix a bug I hit in an earlier version, and am having trouble getting my existing pipeline working given the new config.yml mechanism.


Describe the bug

V1's configuration mechanism sufferers from race conditions and overwrites other tool's config.yml.

General (please complete the following information):

  • AGAT version (e.g. v0.9.0] 1.0.0
  • AGAT installation/use [e.g. Conda, miniconda3, Manual, Container from ...] MANUAL
  • OS: [e.g. macOS, Ubuntu, CentOS] Ubuntu 22.04

To Reproduce

Run the following two blocks in parallel with e.g. snakemake

agat config --expose --tabix
agat_convert_sp_gxf2gxf.pl ...
agat config --expose --no-tabix
agat_convert_sp_gxf2gxf.pl ...

One agat config will overwrite the other, and often both commands with get either --tabix or --no-tabix at random, depending on FS timing. It will also occasionally work as intended, and I'd imagine there are other strange race condition effects that could be observed.

Expected behavior

In order of preference:

  • allow all parser parameters to be overwritten on the CLI, so that one can do e.g. agat_convert_sp_gxf2gxf.pl --tabix --gff ... and so one does not need to do agat config --expose at all
  • allow users to give a specific config file, so one can have e.g. agat-tabix.yml and agat-notabix.yml and pass e.g. agat_convert_sp_gxf2gxf.pl --config agat-tabix.yml
@Juke34
Copy link
Collaborator

Juke34 commented Apr 11, 2023

Hi thank you for using AGAT and thank you for your feedback.

The first solution is what we had before. It was not doable anymore because AGAT contains 72 scripts and the options has to be handle for all of them, so any tiny modification was a nightmare to scatter over all the scripts.

The second solution can be setup but means modifying all the script to add a --config parameter, and modifying the mechanism to create the config file to be named as wished.

In my sense a proper pipeline should run each step in a dedicated folder (as it is handle by e.g. Nextflow) to avoid any of these inconvenience. Would't be better to modify your pipeline to avoid to encounter that type of cases? Using a workflow manager system (WMS) as Nextflow would avoid to encounter that type of problems.

@kdm9
Copy link
Contributor Author

kdm9 commented Apr 11, 2023

I understand the concern, but i find that solution pretty opaque and it would be a disappointment if this can't be fixed in agat.

An honest question (from someone who has not coded with perl in over a decade): can't one "simply" do something like the following in each of the scripts?

my %cli_args = { ... this scripts's options ... };
agat_add_omnisent_parser_args(cli_args); // update cli_args with e.g. {'tabix' -> ...} and the rest of them
my %args = GetOptions(\%cli_args);
my %config = agat_config_from_args(%args);  // parse global config.yml, update the values from the CLI args

In english, have a function that extends a local hash of this script's args with a centrally-defined set of parser args, and another that generates a config from the global set of defaults in the yaml and the CLI args?

It's a pretty common pattern in at least python & C/C++, so I assume this is possible even if my perl code above is a bit off.

@Juke34
Copy link
Collaborator

Juke34 commented Apr 11, 2023

^^ Thank you for your suggestions.
It was not easily feasible in ancien versions of AGAT (things were not standardized between scripts), but much more easy since version 1. You are right we could now make it work like that by adding few lines of code. I will think about it before to release a new version.

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 a pull request may close this issue.

2 participants