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

Add logging about not using async for Sequential and Parallel runners #3424

Merged
merged 6 commits into from
Dec 15, 2023

Conversation

merelcht
Copy link
Member

Description

Closes #2405

Development notes

Added logging message only to SequentialRunner and ParallelRunner because async mode can't be used with ThreadRunner. Added tests to check the message is shown/not shown when expected.

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Copy link
Contributor

@DimedS DimedS left a comment

Choose a reason for hiding this comment

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

@merelcht , thank you for this PR,
I noticed a little bit of code repetition in 2 classes, maybe we need to consider to move this common code to AbstractRunner class, like:

if isinstance(self, (SequentialRunner, ParallelRunner)) and not self._is_async:
            self._logger.info(...

@merelcht
Copy link
Member Author

@merelcht , thank you for this PR, I noticed a little bit of code repetition in 2 classes, maybe we need to consider to move this common code to AbstractRunner class, like:

if isinstance(self, (SequentialRunner, ParallelRunner)) and not self._is_async:
            self._logger.info(...

It's a good point, but then I was thinking the AbstractRunner ideally shouldn't have any logic specific to the implementations, so that's why I did it on the individual classes.

@DimedS
Copy link
Contributor

DimedS commented Dec 14, 2023

@merelcht , thank you for this PR, I noticed a little bit of code repetition in 2 classes, maybe we need to consider to move this common code to AbstractRunner class, like:

if isinstance(self, (SequentialRunner, ParallelRunner)) and not self._is_async:
            self._logger.info(...

It's a good point, but then I was thinking the AbstractRunner ideally shouldn't have any logic specific to the implementations, so that's why I did it on the individual classes.

you right, it's ambiguous trade-off, we can leave it as it is 👍

merelcht and others added 2 commits December 15, 2023 13:49
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
kedro/runner/sequential_runner.py Outdated Show resolved Hide resolved
tests/runner/test_parallel_runner.py Outdated Show resolved Hide resolved
tests/runner/test_sequential_runner.py Outdated Show resolved Hide resolved
tests/runner/test_thread_runner.py Outdated Show resolved Hide resolved
Signed-off-by: Merel Theisen <[email protected]>
@merelcht merelcht enabled auto-merge (squash) December 15, 2023 16:13
@merelcht merelcht merged commit 67cadc2 into main Dec 15, 2023
36 checks passed
@merelcht merelcht deleted the update-async-logs branch December 15, 2023 17:09
AhdraMeraliQB pushed a commit that referenced this pull request Dec 19, 2023
AhdraMeraliQB added a commit that referenced this pull request Dec 20, 2023
* unique tool entires and QoL for pyproject.toml

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* revert unique entries change

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Update cli.py template (#3428)

Signed-off-by: Ahdra Merali <[email protected]>

* Add logging about not using async for Sequential and Parallel runners (#3424)

Signed-off-by: Merel Theisen <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Don't include requirements only needed for example pipeline (#3425)

* remove example pipeline requirements

Signed-off-by: Sajid Alam <[email protected]>

* lint

Signed-off-by: Sajid Alam <[email protected]>

* simplify amending kedro[...] lines

Signed-off-by: Sajid Alam <[email protected]>

* keep the version for datasets

Signed-off-by: Sajid Alam <[email protected]>

* lint

Signed-off-by: Sajid Alam <[email protected]>

---------

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Sort `requirements.txt` based on package name only (#3436)

* Sort `requirements.txt` based on package name only

Signed-off-by: Deepyaman Datta <[email protected]>

* Remove now-unused custom `requirements.txt` sorter

Signed-off-by: Deepyaman Datta <[email protected]>

* Ruff format kedro/templates/project/hooks/utils.py

Signed-off-by: Deepyaman Datta <[email protected]>

* Pass the right argument to `fix_requirements` call

Signed-off-by: Deepyaman Datta <[email protected]>

* Add `sort_requirements` until starters are updated

Signed-off-by: Deepyaman Datta <[email protected]>

* Wrap lib call in existing method for compatibility

Signed-off-by: Deepyaman Datta <[email protected]>

---------

Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Pass tools through as list

Signed-off-by: Ahdra Merali <[email protected]>

* Revert "Pass tools through as list"

This reverts commit f4a4a15.

Signed-off-by: Ahdra Merali <[email protected]>

* Remove duplicates

Signed-off-by: Ahdra Merali <[email protected]>

* Fix for no add-ons selected

Signed-off-by: Ahdra Merali <[email protected]>

* Lint

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt2

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt3

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt4

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt5

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt6

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt7

Signed-off-by: Ahdra Merali <[email protected]>

* Streamline condition

Signed-off-by: Ahdra Merali <[email protected]>

---------

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Co-authored-by: Ahdra Merali <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Co-authored-by: Deepyaman Datta <[email protected]>
Co-authored-by: Ahdra Merali <[email protected]>
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.

Inform users about not using --async
3 participants