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

[Tools] Parallel integration #289

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

[Tools] Parallel integration #289

wants to merge 13 commits into from

Conversation

ebosnjak
Copy link
Collaborator

This PR improves the run_integration.py script by running integration tests in parallel, using Python's ProcessPoolExecutor.

By doing this, performance is significantly improved: local testing shows that all tests (besides the ones in ignored_tests.txt) are completed in around 4 minutes, unlike the current performance of the CI/CD workflow, which takes over an hour.

@ebosnjak ebosnjak linked an issue Feb 12, 2025 that may be closed by this pull request
@pcineverdies
Copy link
Collaborator

Wow, 12 minutes to run, that's amazing! Thanks :)
I would add this PR (maybe it's already present - I'll go through the review in the next few days) what was suggested here regarding the need of a more verbose output for failing tests. Is there anything we can do? Maybe, rather than printing the content of report.txt on the output, you can find a way to show a separate file containing the content of each report.txt for the failing tests.

(not only report.txt, but also the output of ./bin/dynamatic --run xxx, in case the tests fails due to compilation problems)

Sorry for the chaotic message, I wanted to give you a quick feedback first! :)

@ebosnjak
Copy link
Collaborator Author

I can use actions/upload-artifact to upload whatever you want as an artifact that you would be able to download after running the job. This can include full reports from all tests and compilation results etc. Would you be okay with that?

@pcineverdies
Copy link
Collaborator

That sounds great! I think that only what's failing should be kept, but maybe I am wrong. I guess that the more the better, in this sense. I would keep report.txt and the output from Dynamic binary. I cannot come up with anything else useful...

@ebosnjak
Copy link
Collaborator Author

I made it upload the report.txt and Dynamatic's output (stdout and stderr) as you said. Let me know what you think!

Copy link
Collaborator

@pcineverdies pcineverdies left a comment

Choose a reason for hiding this comment

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

Just a couple of minor things, everything else looks good :)

tools/integration/run_integration.py Outdated Show resolved Hide resolved
tools/integration/run_integration.py Outdated Show resolved Hide resolved
tools/integration/run_integration.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@shundroid shundroid left a comment

Choose a reason for hiding this comment

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

Thank you for implementing parallel execution! I'm also thrilled by the speedup!

tools/integration/run_integration.py Outdated Show resolved Hide resolved
Comment on lines 39 to 42
self.parser.add_argument(
"-w", "--workers",
help="Number of workers to run in parallel for testing. Default is os.cpu_count()."
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work?

        self.parser.add_argument(
            "-w", "--workers",
            nargs="?",
            type=int
            help="Number of workers to run in parallel for testing. Default is os.cpu_count()."
        )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it does, thanks! I changed it for the timeout argument as well.

Comment on lines 335 to 337
workers = None
if args.workers:
workers = int(args.workers)
Copy link
Collaborator

Choose a reason for hiding this comment

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

then you don't need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed

Comment on lines 36 to 37
"-t", "--timeout",
help="Custom timeout value for a single test. If not given, 500 seconds is used."
Copy link
Collaborator

Choose a reason for hiding this comment

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

also can you specify the default timeout time here?
then you can remove the description from help

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the argument default=500, but this does not automatically add it to the help description if I remove it.

Comment on lines 303 to 305
# If timeout is not given, use default of 500 seconds
timeout = int(args.timeout or 500)

Copy link
Collaborator

Choose a reason for hiding this comment

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

then you don't need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Parallelize Integration Tests
3 participants