Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add matrix strategy #118

Merged
merged 7 commits into from
Oct 15, 2024
Merged

Conversation

abhijeetSaroha
Copy link
Collaborator

Add support of matrix.

Solves #81

@abhijeetSaroha
Copy link
Collaborator Author

While the combination logic successfully creates all 6 combinations, the output seems to be referencing the matrix variables without properly substituting their values. The output looks like this for each combination:

[Setup] Environment: {{ matrix.env }}, Architecture: {{ matrix.arch }}
[Setup] Using base image: {{ vars.base_image }}
[Setup] Installing dependencies for {{ matrix.env }}

Tried using ${{ matrix.env }} or ${{ matrix.arch }} for variable substitution, but this resulted in a "matrix variable not found" error.

Additionally, commands like makim tests.smoke that were previously functional are no longer running after this change.

@abhijeetSaroha abhijeetSaroha requested a review from xmnlab October 10, 2024 14:27
@xmnlab
Copy link
Member

xmnlab commented Oct 11, 2024

hi @abhijeetSaroha, thanks for working on that! it seems it is almost ready to go :)

the jinja template is defined as ${{ ... }} as defined here: https://github.com/osl-incubator/makim/blob/main/src/makim/core.py#L51

it fails because it is trying to process, but there are some errors inside the code.

check all the usage of TEMPLATE in the core.py, for example here: https://github.com/osl-incubator/makim/blob/main/src/makim/core.py#L379

as you can see there, it sets the variables for env and for vars, you need to add it for matrix as well.

let me know if you need any more details please

@abhijeetSaroha
Copy link
Collaborator Author

It works fine now as it should be.

I also break the function _run_command in two functions as you said. I have create another function for MakimLogs execution.

https://github.com/abhijeetSaroha/makim/blob/matrix-strategy/src/makim/core.py#L455C2-L480C42

Copy link
Member

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abhijeetSaroha , thanks for working on that.

in general it looks good to me .. just a few changes, I will also paste here the complete diff:

diff --git a/src/makim/core.py b/src/makim/core.py
index 06e1f7a..cd6fb11 100644
--- a/src/makim/core.py
+++ b/src/makim/core.py
@@ -33,6 +33,7 @@ from makim.logs import MakimError, MakimLogs
 
 AppConfigType: TypeAlias = Dict[str, Union[str, List[str]]]
 
+
 SCOPE_GLOBAL = 0
 SCOPE_GROUP = 1
 SCOPE_TARGET = 2
@@ -495,7 +496,7 @@ class Makim:
             List of dictionaries, each containing one possible combination
         """
         if not matrix_config:
-            return [{}]
+            return []
 
         # Convert matrix config into format suitable for product
         keys = list(matrix_config.keys())
@@ -588,7 +589,7 @@ class Makim:
             )
 
         # Get matrix configuration if it exists
-        matrix_combinations: list[dict[str, Any]] = [{}]
+        matrix_combinations: list[dict[str, Any]] = []
         if matrix_config := self.task_data.get('matrix', {}):
             matrix_combinations = self._generate_matrix_combinations(
                 matrix_config
@@ -631,7 +632,7 @@ class Makim:
         width, _ = get_terminal_size()
 
         # Run command for each matrix combination
-        for matrix_vars in matrix_combinations:
+        for matrix_vars in matrix_combinations or [{}]:
             # Update environment variables
             for k, v in env.items():
                 os.environ[k] = v

also please rebase your branch so you can get the changes that fixes the linter on ci.

for k, v in env.items():
os.environ[k] = v
# Get matrix configuration if it exists
matrix_combinations: list[dict[str, Any]] = [{}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
matrix_combinations: list[dict[str, Any]] = [{}]
matrix_combinations: list[dict[str, Any]] = []

List of dictionaries, each containing one possible combination
"""
if not matrix_config:
return [{}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return [{}]
return []

MakimLogs.print_info(
'TARGET: ' + f'{self.group_name}.{self.task_name}'
# Run command for each matrix combination
for matrix_vars in matrix_combinations:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for matrix_vars in matrix_combinations:
for matrix_vars in matrix_combinations or [{}]:

@abhijeetSaroha
Copy link
Collaborator Author

@xmnlab, I have made the changes and rebase the branch too.

@xmnlab xmnlab merged commit 89894cf into osl-incubator:main Oct 15, 2024
14 checks passed
@xmnlab
Copy link
Member

xmnlab commented Oct 15, 2024

thank you @abhijeetSaroha ! great job!

Copy link

🎉 This PR is included in version 1.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@abhijeetSaroha abhijeetSaroha deleted the matrix-strategy branch October 18, 2024 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants