refactor: using blacklist approach instead of whitelist for launch args

[raph-luc]

Suggestion for #2822, from #2822 (comment), merges into that PR

Additional benefit is that it becomes easier to be aware of which arguments exactly are not supported by each launcher, just by looking at the code (work to be done to add support for some of these unsupported arguments?).

Edit: @hpohekar unintentionally exposed another reason to use this suggested approach instead of the current implementation, see: #2826 (comment), in addition to issues where create_launcher() isn't currently set up to be directly exposed to users (unrelated to these changes, issue tracker: #2827).

[prmukherj]

@raph-luc, regarding this PR, though the code looks compact, however, I think getting a view of the arguments specific to the type of the launcher at this level is good (instead of the unsupported ones).
What do you think @seanpearsonuk, @mkundu1, @hpohekar

[raph-luc]

@prmukherj If we want to check which arguments are valid for each launcher by looking at the code, we can easily look at the already existing launcher __init__() methods instead to get the full list. However, we do not have the unsupported arguments easily listed anywhere that I've seen (outside of my suggestion here).

Edit: And I don't think this is a particularly important point by the way, what I mentioned in the description is just an additional benefit, not the main reason to do this.

[hpohekar]

@raph-luc

I have identified this issue.

>>> from ansys.fluent.core.launcher import create_launcher
>>> from ansys.fluent.core.launcher import LaunchMode
>>> solver = create_launcher(LaunchMode.STANDALONE)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "D:\pyfluent\src\ansys\fluent\core\launcher\launcher.py", line 61, in create_launcher
    if kwargs["dry_run"]:
KeyError: 'dry_run'

Then if we update kwargs["dry_run"]: to kwargs.get("dry_run"): then we get following error.

>>> from ansys.fluent.core.launcher import create_launcher
>>> from ansys.fluent.core.launcher import LaunchMode
>>> solver = create_launcher(LaunchMode.STANDALONE)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "D:\pyfluent\src\ansys\fluent\core\launcher\launcher.py", line 65, in create_launcher
    mode=kwargs["mode"],
KeyError: 'mode'

For each optional arguments we get key error due to use of kwargs[<argument_name>] if it is not provided by the user.

[raph-luc]

@hpohekar indeed, part of that is exactly what I said here: #2826 (comment). I see similar issues in your branch fix/launcher_argvals_refactoring:

>>> import ansys.fluent.core as pf
>>> from ansys.fluent.core.launcher.pyfluent_enums import LaunchMode
>>> pf.launcher.launcher.create_launcher(LaunchMode.STANDALONE) 
Traceback (most recent call last):
  File "C:\ansysdev\pyfluent\src\ansys\fluent\core\launcher\launcher.py", line 61, in create_launcher
    _process_invalid_args(kwargs["dry_run"], fluent_launch_mode, kwargs)
KeyError: 'dry_run'

The root issue here is not related to my changes. As I already mentioned here: #2826 (comment), if the intention is to expose create_launcher() directly to users, there is some work that we still need to do so that it works properly for the various launch modes. I think these issues should be addressed in a separate PR as they already existed before these changes.

Edit: created an issue tracker so that we don't forget to address these issues: #2827

[hpohekar]

@hpohekar indeed, part of that is exactly what I said here: #2826 (comment). I see similar issues in your branch fix/launcher_argvals_refactoring:

>>> import ansys.fluent.core as pf
>>> from ansys.fluent.core.launcher.pyfluent_enums import LaunchMode
>>> pf.launcher.launcher.create_launcher(LaunchMode.STANDALONE) 
Traceback (most recent call last):
  File "C:\ansysdev\pyfluent\src\ansys\fluent\core\launcher\launcher.py", line 61, in create_launcher
    _process_invalid_args(kwargs["dry_run"], fluent_launch_mode, kwargs)
KeyError: 'dry_run'

The root issue here is not related to my changes. As I already mentioned here: #2826 (comment), if the intention is to expose create_launcher() directly to users, there is some work that we still need to do so that it works properly for the various launch modes. I think these issues should be addressed in a separate PR as they already existed before these changes.

Edit: created an issue tracker so that we don't forget to address these issues: #2827

@raph-luc I will look into this issue. I have resolved this locally by using whitelist approach but we will merge this PR first then I will create a separate PR to address this. Thank you.

[mkundu1]

I'm generally fine with both approaches, I think both are equivalent w.r.t. catching unsupported arguments. The only thing about the blacklist approach, for example if I add a new argument for slurm launcher, I need to remember to exclude it from all other launchers (the testing requirement will not be isolated to my change).

[raph-luc]

Thanks @mkundu1, I think that is a fair point, though I don't think that is the best example as we already have a dict scheduler_options for the SlurmLauncher; there'd be no reason to add a new SlurmLauncher argument compared to just adding it to scheduler_options, in my understanding.

Regardless, I would raise the counterpoint that in general most launcher arguments should work for all launchers, do you agree/disagree? Outside of the obviously specific arguments such as container_dict and scheduler_options, all/most of the others should ideally work for all launchers in my opinion. For example, shouldn't we support the specification of journal_file_names, case_file_name, case_data_file_name, etc., for the DockerLauncher as well? Same thing for the others.

It follows that if we are adding a new argument (that I would expect is most of the time going to be relevant for all launchers), we should support it in all launchers, otherwise we will indeed need to specify which launchers do not support that new argument, which seems like good practice to me.

Does this make sense? Let me know if you prefer to discuss this in a meeting/call, we can discuss next week as well. Regardless, this was just a quick suggestion that is already resulting in way more discussion than I ever imagined, feel free to just disregard if you prefer.

Note also that the SlurmLauncher seems to be defined differently from the other launchers currently, and is just ignoring unsupported arguments with no warnings or anything of the sort.