Add _translate_constraint_by_config function

[skycastlelily]

Pull Request Checklist

• [x] implement the feature
• [ ] write the documentation
• [x] extend the test coverage
• [ ] update the specification
• [ ] adjust plugin docstring
• [ ] modify the json schema
• [ ] mention the version
• [ ] include a release note

[skycastlelily]

This MR looks at config file and tries to find a "translation" that would match the constraint, operator, value, and if found, it'd take it from configuration,we will need this to translate some complex or containing-internal-information constrains :)

[skycastlelily]

yeah,it will be used if any transform__ function calls _translate_constraint_by_config,should I move this function to a new merge request?^^

On Mon, Jun 3, 2024 at 5:01 PM Miloš Prchlík @.***> wrote:

@.**** commented on this pull request.

In tmt/steps/provision/mrack.py https://github.com/teemtee/tmt/pull/2967#discussion_r1624069253:

@@ -197,6 +202,34 @@ class MrackHWNotGroup(MrackHWGroup): name: str = 'not'

+def _get_custom_config() -> dict[str, Any]:

This function does not seem to be called from anywhere.

— Reply to this email directly, view it on GitHub https://github.com/teemtee/tmt/pull/2967#pullrequestreview-2093240783, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23EQLS4CPESTI3BWEEDZFQWFPAVCNFSM6AAAAABIOH3ASGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJTGI2DANZYGM . You are receiving this because you authored the thread.Message ID: @.***>

[skycastlelily]

Updated^^

[skycastlelily]

Thanks for your super quick response,and pretty valuable suggestions, updated accordingly^^

[happz]

Adding blocked, to get https://github.com/teemtee/tmt/pull/3519 merged first as it simplifies some pieces of configuration handling.

[happz]

BTW the PR deserves a better subject and message.

[skycastlelily]

BTW the PR deserves a better subject and message.

Updated,WDYT?^^ https://github.com/teemtee/tmt/pull/2967/commits/7783dbb4b5ae7a116acf94f4e04bbede2ca06a09

[skycastlelily]

Thanks for working on this.

Pleasure, thanks for your review.

I'm proposing a small enhancement in https://github.com/teemtee/tmt/commit/c20edbd6b4f7f003cd9b492d01db607bba422996. Does it look ok?

Yeah, thanks:)

The pull request check list states that test coverage has been extended, but there seem to be no changes to tests. What >about adding a simple test exercising just tmt provision --dry to ensure everything works as expected?

I checked that list by mistake,sorry for the confusing, do you see where we could add the test? or, maybe you could extend the test for us if you are not too busy? :saluting_face: :grin: You should know we need a hardware
translated by hardware.py but not translate by mrack.py, for instance, boot.method. Because if the constraint has a _CONSTRAINT_TRANSFORMERS, the _translate_constraint_by_config method will not be called.

Ah, wait, after think twice, maybe we should let custom config surpass the default one in mrack.py, ie ,we check whether if there is specific translates in config file first, if not, use _CONSTRAINT_TRANSFORMERS. WDYT?:) @happz

Just to clarify the motivation: So users will be able to configure (and maintain) their own hardware translation rules?

yep

Do we expect this will be a common use case? Do we expect something similar for other provision plugins as well?

I guess both of answer is "yes" and @happz may be able to offer more info:)

I tried a simple modification of your config example but it does not seem to be working as expected. Am I doing >anything wrong?

That is because hardware.py does not support gpu.processors ,you need add processors to number_constraints. @happz mentor, I remembered you add the gpu translation and _parse_device_core ,do you think we should add support processors and maybe frequency for gpu? Smells like a mr for me; You also need to change VALUE to BEAKER_VALUE in hardware.fmf to have it fully work.

[psss]

Ah, wait, after think twice, maybe we should let custom config surpass the default one in mrack.py,

Yeah, this sounds reasonable to me: To allow users override default translation with their custom settings.

That is because hardware.py does not support gpu.processors ,you need add processors to number_constraints.

So there are additional actions needed from the user to enable a specific translation?

I checked that list by mistake,sorry for the confusing, do you see where we could add the test? or, maybe you could extend the test for us if you are not too busy?

Sorry, no extra time as for now. But I'd say a very simple translation example and check with provision --dry would be completely sufficient.

[happz]

Ah, wait, after think twice, maybe we should let custom config surpass the default one in mrack.py,

Yeah, this sounds reasonable to me: To allow users override default translation with their custom settings.

It wasn't meant to step in before the hardcoded ones, but rather to close the gap where the hardcoded one does not exist yet or never should, but I think it makes perfect sense to inspect the translations first, then fall back to hardcoded translations, then give up.

That is because hardware.py does not support gpu.processors ,you need add processors to number_constraints.

So there are additional actions needed from the user to enable a specific translation?

Yes and no. You tried to translate a HW requirement that is not yet defined by tmt specification. As such, it is not parsed by tmt, and it is not delivered to beaker plugin to inspect and convert into Beaker XML. So in this sense, yes, there are additional actions needed, define the constraint and add its support to tmt.hardware.

For already-defined constraints, there should be no other action needed. The goal is to give beaker a chance to use more complicated, deployment-specific XML snippets instead of hardcoded ones - our current downstream filter for virtualization.is-virtualized is many, many lines long and depends on our Beaker's setup. So, gpu.processors no, gpu.driver yes, that should manifest the functionality.

[happz]

Just to clarify the motivation: So users will be able to configure (and maintain) their own hardware translation rules?

yep

Do we expect this will be a common use case? Do we expect something similar for other provision plugins as well?

I guess both of answer is "yes" and @happz may be able to offer more info:)

Yes, and yes: users - and this means also TF - should be able to add their own custom translations to be used with Beaker. Some of them are very complicated, depend on the current setup of our Beaker deployment and it just does not feel right to hardcode these. So the idea, taken from Artemis, is to allow configuration to supply anything that goes beyond trivial translation of requirements like cpu.cores.

I do expect something similar for other plugins: Artemis works a lot with instance-type-based clouds, and while it's not the case of artemis plugin, minute is a prime example of this feature: what OpenStack flavor should be picked for --how minute --memory '>= 4GB'? Artemis keeps a list of instance types and their properties, updated dynamically, for minute and other plugins we may add in the future I envision such a list to exist in configuration. Just like we have a config file for mrack, with distros and distro variants, for example. Our qe-tools-workstation can easily ship something like ~/.config/tmt/hardware/minute/main.fmf with a list of available flavors.

I tried a simple modification of your config example but it does not seem to be working as expected. Am I doing >anything wrong?

That is because hardware.py does not support gpu.processors ,you need add processors to number_constraints. @happz mentor, I remembered you add the gpu translation and _parse_device_core ,do you think we should add support processors and maybe frequency for gpu? Smells like a mr for me; You also need to change VALUE to BEAKER_VALUE in hardware.fmf to have it fully work.

Nope, no need to add gpu.processors, I believe this is unfortunately a wrong example, no plugin supports gpu.processors. gpu.driver would be better, as it's already defined by the specification.

[psss]

Thanks for the detailed clarification. I've shortly tested with gpu.driver and it seems to be working:

This config:

beaker:
    translations:
      - requirement: gpu.driver
        template: '{"gpu": {"driver": {{ VALUE }} }}'

Together with the plan:

provision:
    how: beaker
    hardware:
        gpu:
            driver: something
execute:
    how: tmt

Gives this:

<hostRequires>
  <and>
    <gpu>
      <driver/>
    </gpu>
  </and>
</hostRequires>

Any quick hint what the template syntax should look like to get the value into the resulting xml?

[happz]

Thanks for the detailed clarification. I've shortly tested with gpu.driver and it seems to be working:

This config:

beaker:
    translations:
      - requirement: gpu.driver
        template: '{"gpu": {"driver": {{ VALUE }} }}'

Together with the plan:

provision:
    how: beaker
    hardware:
        gpu:
            driver: something
execute:
    how: tmt

Gives this:

<hostRequires>
  <and>
    <gpu>
      <driver/>
    </gpu>
  </and>
</hostRequires>

Any quick hint what the template syntax should look like to get the value into the resulting xml?

BEAKER_VALUE instead of VALUE.

I hope to get the documentation in the "generate docs for config from models" batch of PRs, the syntax and examples should be coming from that place.

[psss]

I hope to get the documentation in the "generate docs for config from models" batch of PRs, the syntax and examples should be coming from that place.

Great, thanks!

[skycastlelily]

Hi Petr, did you merge this by mistake? :)

I guess by "but I think it makes perfect sense to inspect the translations first, then fall back to hardcoded translations, then give up." Milos has the same opinion with you, ie, "let custom config surpass the default one in mrack.py" I already modify the code accordingly and halfway in extending the tests, then I went for dinner,and enjoy the leisure time, and then I found this mr is merged :sweat_smile: ,should I send another mr to update the code and extend the test?:)

Below is the draft left in the web page :sweat_smile:

So there are additional actions needed from the user to enable a specific translation?

Just as Milos said, users need to ask for defining the constraint and adding its support to tmt.hardware This patch does not mean to overpass hardware.py .

Sorry, no extra time as for now. But I'd say a very simple translation example and check with provision --dry would be completely sufficient.

yeah, I guess so, just asked to make sure.

[psss]

Hi Petr, did you merge this by mistake? :)

I was just trying to push forward as much as possible before the release and thought that the missing stuff is not strictly blocking this. Also Miloš mentioned there are already some other pull requests with the planned documentation.

I guess by "but I think it makes perfect sense to inspect the translations first, then fall back to hardcoded translations, then give up." Milos has the same opinion with you, ie, "let custom config surpass the default one in mrack.py" I already modify the code accordingly and halfway in extending the tests, then I went for dinner,and enjoy the leisure time, and then I found this mr is merged 😅

Oh, I see. Sorry for that, Lili! 🫣

should I send another mr to update the code and extend the test?:)

Yes, please. If you have the tests extension prepared, let's definitely include it.

I guess by "but I think it makes perfect sense to inspect the translations first, then fall back to hardcoded translations, then give up." Milos has the same opinion with you, ie, "let custom config surpass the default one in mrack.py"

I don't have a strong opinion on the exact order. But in general, yes it probably makes sense to give the user possibility to override hardcoded translations. So that approach sounds probably like that best option.

[skycastlelily]

I was just trying to push forward as much as possible before the release

I guessed so:)

Oh, I see. Sorry for that, Lili! 🫣

no problem,Petr:)

Yes, please. If you have the tests extension prepared, let's definitely include it.

Here is the mr,including the tests and the refactor:) https://github.com/teemtee/tmt/pull/3623