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

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht merelcht self-assigned this Dec 14, 2023
Copy link
Member

@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
Member

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 <juan_luis_cano@mckinsey.com>
Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht merelcht enabled auto-merge (squash) December 15, 2023 16:13
@merelcht merelcht merged commit 67cadc2 into main Dec 15, 2023
@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
…#3424)

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
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 <sajid_alam@mckinsey.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* revert unique entries change

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Update cli.py template (#3428)

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

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

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

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

* remove example pipeline requirements

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>

* lint

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>

* simplify amending kedro[...] lines

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>

* keep the version for datasets

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>

* lint

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>

---------

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

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

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

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

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

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

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

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

* Pass the right argument to `fix_requirements` call

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

* Add `sort_requirements` until starters are updated

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

* Wrap lib call in existing method for compatibility

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

---------

Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Pass tools through as list

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Revert "Pass tools through as list"

This reverts commit f4a4a15.

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Remove duplicates

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Fix for no add-ons selected

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Lint

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Add example as recognised key

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Add example as recognised key pt2

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Add example as recognised key pt3

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Add example as recognised key pt4

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Add example as recognised key pt5

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Add example as recognised key pt6

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Add example as recognised key pt7

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Streamline condition

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

---------

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Sajid Alam <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Co-authored-by: Ahdra Merali <90615669+AhdraMeraliQB@users.noreply.github.com>
Co-authored-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>
Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Co-authored-by: Ahdra Merali <ahdra.merali@quantumblack.com>
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