ROCm / MITuna

MIT License
7 stars 0 forks source link

Generic Import Configs for Tuna's subcmds #931

Closed reidkwja closed 10 months ago

reidkwja commented 11 months ago

This PR allows future implementations (similar to implemented MIOpen subcmd changes) for sub-args and creates a generic parser for import configs.

alexandraBara commented 10 months ago

After looking at the tuna/mlir implementation and these changes here, I am not sure that there is much value in adding generic parser functions without somehow enforcing their use. I think a better design would be something like a parser object that is then specialized for: import_configs, load_jobs and is defined by your library. I am leaving this thought here so we can discuss more offline.

cderb commented 10 months ago

After looking at the tuna/mlir implementation and these changes here, I am not sure that there is much value in adding generic parser functions without somehow enforcing their use. I think a better design would be something like a parser object that is then specialized for: import_configs, load_jobs and is defined by your library. I am leaving this thought here so we can discuss more offline.

I had a similar idea that the parser object would be like the driver class that exists in the miopen directory. If a new module implements a line parser like the driver in miopen, then the import_configs script could be generically applicable.

reidkwja commented 10 months ago

Will the genericized import_configs.py be addressed in a follow-up PR?

@cderb @alexandraBara Yes, I've planned it out in a way that will address the generic implementation in multiple sub-tasks to reduce PR complexity. I agree that we perhaps might need to either address the MLIR + other subcm libraries if we decided to proceed forward.

alexandraBara commented 10 months ago

After looking at the tuna/mlir implementation and these changes here, I am not sure that there is much value in adding generic parser functions without somehow enforcing their use. I think a better design would be something like a parser object that is then specialized for: import_configs, load_jobs and is defined by your library. I am leaving this thought here so we can discuss more offline.

I had a similar idea that the parser object would be like the driver class that exists in the miopen directory. If a new module implements a line parser like the driver in miopen, then the import_configs script could be generically applicable.

I would agree with such a design, this would allow for a 'generic' import_configs with specialized parsers as @cderb mentioned, in the subclasses. I don't like just breaking up the script and pulling out functions, these functions need to be tied to a class somehow otherwise we cannot enforce their use and every new user could/would have to - just duplicate the work, as it seems like mlir is doing.

alexandraBara commented 10 months ago

Will the genericized import_configs.py be addressed in a follow-up PR?

@cderb @alexandraBara Yes, I've planned it out in a way that will address the generic implementation in multiple sub-tasks to reduce PR complexity. I agree that we perhaps might need to either address the MLIR + other subcm libraries if we decided to proceed forward.

Let's discuss in a meeting, i think we need to backtrack a little.

cderb commented 10 months ago

I would agree with such a design, this would allow for a 'generic' import_configs with specialized parsers as @cderb mentioned, in the subclasses. I don't like just breaking up the script and pulling out functions, these functions need to be tied to a class somehow otherwise we cannot enforce their use and every new user could/would have to - just duplicate the work, as it seems like mlir is doing.

With this change I'd like for modules like mlir to be able to write their 'driver' which could parse a file containing the module's configurations. There are some extra flags for parsing in import_configs.py currently which need to be move to a specialization, but the core of import configs could be adapted as something that is generally useable.

End goal: new modules would only write the 'driver', to perform file parsing for configurations. To accomplish this import_configs.py will also need to have its generic features split from its miopen ones.

Of note is that the miopen specific features in import_configs.py are actually supporting tables and not the config table itself.

reidkwja commented 10 months ago

Using a different approach