@pplantinga

Hi @nouranali - thanks for your contribution. This is a step in the right direction, but I think there's more that's needed here. Just to warn you, the core.py and interfaces.py are important parts of the code and will need a lot of testing before this sort of PR can be merged.

So to start with, I think Brain and Pretrained should accept a RunOpts object as an argument, rather than a dict. In addition, the parse_arguments() function is closely tied to the RunOpts, and should maybe just be a method of the class or at least in the same file, and should probably just return a RunOpts object (e.g. RunOpts.from_command_line_args() @classmethod or some such thing). Finally, there is some argument validation that is done in Brain that could maybe move to the RunOpts class. Use your best judgement -- this part is probably the tricky part of the PR, as we should focus on making the Brain class an easy to understand basic training loop implementation, while no surprising details should get hidden if possible. Good luck!

Hi @nouranali - thanks for your contribution. This is a step in the right direction, but I think there's more that's needed here. Just to warn you, the core.py and interfaces.py are important parts of the code and will need a lot of testing before this sort of PR can be merged.

So to start with, I think Brain and Pretrained should accept a RunOpts object as an argument, rather than a dict. In addition, the parse_arguments() function is closely tied to the RunOpts, and should maybe just be a method of the class or at least in the same file, and should probably just return a RunOpts object (e.g. RunOpts.from_command_line_args() @classmethod or some such thing). Finally, there is some argument validation that is done in Brain that could maybe move to the RunOpts class. Use your best judgement -- this part is probably the tricky part of the PR, as we should focus on making the Brain class an easy to understand basic training loop implementation, while no surprising details should get hidden if possible. Good luck!

Great, will start working on your comments, how can I start testing along the way?

@pplantinga Can u check the new updates?

I don't have a lot of time to review this rn, but you can check it yourself by running a template, e.g. templates/speech_recognition/ASR

My only comments right now are: the from_command_line_args() method should be a @classmethod. And ideally the hparams file and yaml_overrides should become members of the RunOpts. I think the name RunOptDefaults should be changed to something like RunOptions.

https://docs.python.org/3/library/functions.html#classmethod

My only comments right now are: the from_command_line_args() method should be a @classmethod. And ideally the hparams file and yaml_overrides should become members of the RunOpts. I think the name RunOptDefaults should be changed to something like RunOptions.

https://docs.python.org/3/library/functions.html#classmethod

Done

@mravanelli

@mravanelli

I ran the checks and the linter failed. You can run it yourself, even automatically on every commit, following instructions here:
https://github.com/speechbrain/speechbrain/blob/develop/docs/devtools.md

At this point I'm unsure if this PR will need to change every recipe or not -- ideally we could get away without having to do this, but we'll see. Have you tried running a template yet?

yes

At this point I'm unsure if this PR will need to change every recipe or not -- ideally we could get away without having to do this, but we'll see. Have you tried running a template yet?

Yes I ran this
https://github.com/speechbrain/speechbrain/tree/develop/templates/speech_recognition/ASR

This used to touch only a few files but now touches over 500... did you run a different version of black on the whole repo?

This used to touch only a few files but now touches over 500... did you run a different version of black on the whole repo?

What version do u use? and what files do u run it on?

If you install the precommit hook, the linters will run on all the files changed as part of the commit. The versions are in the file https://github.com/speechbrain/speechbrain/blob/develop/lint-requirements.txt

@pplantinga / @Adel-Moumen if agreement from both of you, please merge!