@tamarl08 you'll need to rebase.

@mulhod I see your point. I also don't like the fact that it defaults to 1, but can get a value of None to enable parallelization.

I didn't find anything else that we control using an environment variable, do we do that?

Another option I thought of, is adding a general option of sending extra skll args dict in the call to run_experiment. It will also allow disabling the grid search and specifying fixed parameters, and possibly many other tuning options, without making rsmtool config look like skll config.

Honestly, I am not that bothered by the existence of this parameter since it's meant for advanced users who will probably already be familiar with the same parameter in SKLL.

I am not sure I see the benefits of using an environment variable over a configuration setting since then users have to think about two places rather than one for settings.

I think the idea of being able to pass a dictionary is great for advanced users but we should make that a longer term goal rather than for this PR.

I think what I was perhaps envisioning was something like:

Get environment variable "SKLL_GRID_JOBS" if it exists; if not, use "grid_jobs" from config; if neither, use 1. This way, it could be passed in either way. But, it does make it more complicated.

@mulhod I see your point. I also don't like the fact that it defaults to 1, but can get a value of None to enable parallelization.

I didn't find anything else that we control using an environment variable, do we do that?

Another option I thought of, is adding a general option of sending extra skll args dict in the call to run_experiment. It will also allow disabling the grid search and specifying fixed parameters, and possibly many other tuning options, without making rsmtool config look like skll config.

By the way, this is how it works in skll.

https://github.com/EducationalTestingService/skll/blob/main/skll/utils/constants.py#L138
https://github.com/EducationalTestingService/skll/blob/main/skll/learner/__init__.py#L1093
https://github.com/EducationalTestingService/skll/blob/main/skll/learner/__init__.py#L1857

Codecov Report

Patch coverage is 100.00% of modified lines.

📢 Thoughts on this report? Let us know!.

@mulhod I see your point. I also don't like the fact that it defaults to 1, but can get a value of None to enable parallelization.
I didn't find anything else that we control using an environment variable, do we do that?
Another option I thought of, is adding a general option of sending extra skll args dict in the call to run_experiment. It will also allow disabling the grid search and specifying fixed parameters, and possibly many other tuning options, without making rsmtool config look like skll config.

By the way, this is how it works in skll.

https://github.com/EducationalTestingService/skll/blob/main/skll/utils/constants.py#L138

Honestly, I've never really been a fan of that approach and I think most users of SKLL either don't know about it or use it. It also doesn't seem to be mentioned in the docs at all so it's more of a hidden setting.

@tamarl08 @mulhod since this new setting only affects SKLL models, I think we should prefix it with skll_ to call it skll_grid_search_jobs in keeping with other options like skll_objective that already exist. Thoughts?

@tamarl08 did you already check that automatic and interactive config generation work with this new field?

I added the field to the output and the tests pass, do we usually do manual testing as well?

Yes, I usually like to test the config generation by running rsmtool generate and rsmtool generate --interactive just to be sure.

Yes, I usually like to test the config generation by running rsmtool generate and rsmtool generate --interactive just to be sure.

Done!