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

Remove DeprecatedClassMeta in favor of getattr #2724

Merged
merged 4 commits into from
Jun 28, 2023

Conversation

deepyaman
Copy link
Member

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Development notes

Checklist

  • Read the contributing guidelines
  • 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

@deepyaman deepyaman changed the title Let Remove DeprecatedClassMeta in favor of getattr Jun 26, 2023
@astrojuanlu

This comment was marked as resolved.

@deepyaman deepyaman marked this pull request as ready for review June 27, 2023 07:55
@deepyaman deepyaman requested a review from idanov as a code owner June 27, 2023 07:55
@astrojuanlu
Copy link
Member

Test failures on Windows are unrelated, I guess I'll restart the CI a couple more times...

@astrojuanlu astrojuanlu force-pushed the refactor/remove-deprecatedenummeta branch 2 times, most recently from f84096b to 345f6ac Compare June 27, 2023 14:33
@astrojuanlu
Copy link
Member

Well, I can't reproduce these locally, but the win_e2e_tests are consistently failing every single time, always in the same scenario. Now I added a 120 seconds timeout (locally it takes ~1) and here's what happens:

  Scenario: Execute node convert into Python files                    # features/jupyter.feature:21
    Given I have prepared a config file                               # features/steps/cli_steps.py:143
    And I have run a non-interactive kedro new with starter "default" # features/steps/cli_steps.py:237
    Given I have added a test jupyter notebook                        # features/steps/cli_steps.py:226
    When I execute the test jupyter notebook and save changes         # features/steps/cli_steps.py:361
    And I execute the kedro jupyter command "convert --all"           # features/steps/cli_steps.py:330
    And Wait until the process is finished for up to "120" seconds    # features/steps/cli_steps.py:355
      Traceback (most recent call last):
        File "C:\tools\miniconda3\envs\kedro_builder\lib\site-packages\behave\model.py", line 1329, in run
          match.run(runner.context)
        File "C:\tools\miniconda3\envs\kedro_builder\lib\site-packages\behave\matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
        File "features\steps\cli_steps.py", line 358, in wait
          context.result.wait(timeout)
        File "C:\tools\miniconda3\envs\kedro_builder\lib\subprocess.py", line 1189, in wait
          return self._wait(timeout=timeout)
        File "C:\tools\miniconda3\envs\kedro_builder\lib\subprocess.py", line 1473, in _wait
          raise TimeoutExpired(self.args, timeout)
      subprocess.TimeoutExpired: Command '['C:\\Users\\circleci\\AppData\\Local\\Temp\\tmpujtx_5z6\\Scripts\\kedro', 'jupyter', 'convert', '--all']' timed out after 120 seconds

    Then I should get a successful exit code                          # None
    And Code cell with node tag should be converted into kedro node   # None

I have absolutely no idea how the changes in this branch can interact with that part of the code, but it was working fine on main, and it failed always in the same way every time I restarted the CI here, so there must be something.

@deepyaman
Copy link
Member Author

deepyaman commented Jun 27, 2023

To ensure that dependent repos still pass:

  • 🟢 kedro-viz
    • There's one mypy error that will probably need to be resolved after: package/tests/test_data_access/test_managers.py:233: error: Argument 2 to "isinstance" has incompatible type "AbstractDataSet[Any, Any]"; expected "Union[type, UnionType, Tuple[Union[type, UnionType, Tuple[Any, ...]], ...]]" [arg-type]
  • 🟢 kedro-plugins
  • 🟢 kedro-starters

@noklam
Copy link
Contributor

noklam commented Jun 27, 2023

The e2e test is failing due to timeout, not sure what is the issue yet. More importantly, I think this PR introduced some undesired behaviour.

See the screenshot below, doing any kedro command now will generate many warning message. This isn't the case with current main so I believe this is introduced by the PR.

image

@astrojuanlu
Copy link
Member

That's true, we need to silence the deprecation warnings when importing the classes in our own code:

from .cached_dataset import CachedDataSet, CachedDataset

@astrojuanlu astrojuanlu force-pushed the refactor/remove-deprecatedenummeta branch from 49a2790 to c96ebe4 Compare June 27, 2023 23:46
@noklam
Copy link
Contributor

noklam commented Jun 27, 2023

Is it related? #2446

I have run the test locally in my window laptop and I don't have any problem.

@astrojuanlu
Copy link
Member

Is it related? behave --no-capture -n "Execute node convert into Python files"

I can reproduce after SSH'ing to the CircleCI job, yes. However, running C:\Users\circleci\AppData\Local\Temp\...\Scripts\kedro jupyter convert --all works perfectly fine.

My suspicion is that, since there are more warnings now, the subprocess.Popen calls that our e2e tests use are getting deadlocked:

This will deadlock when using stdout=PIPE or stderr=PIPE and the child process generates enough output to a pipe such that it blocks waiting for the OS pipe buffer to accept more data. Use Popen.communicate() when using pipes to avoid that.

https://docs.python.org/3/library/subprocess.html#subprocess.Popen.wait

I just made the change to .communicate to see if that fixes the issue.

@noklam
Copy link
Contributor

noklam commented Jun 27, 2023

Lookd promising, the convert tests is now passing, but the good old notebook and test started failing.
https://app.circleci.com/pipelines/github/kedro-org/kedro/23719/workflows/16441b18-1657-4d99-afc6-855e7065edbb/jobs/272755

@astrojuanlu
Copy link
Member

Yep, I fixed one, but broke another. I consider it a success nonetheless 😂

In the short term, we should reduce the extra warnings to hopefully avoid the I/O deadlock. After that (in another PR) we might need to leave a note about reworking our e2e tests again. Supposedly subprocess.run takes care of killing the child processes.

But now... 💤

@astrojuanlu

This comment was marked as off-topic.

@noklam

This comment was marked as off-topic.

@astrojuanlu
Copy link
Member

win_e2e tests are passing, will now go to sleep for real

@astrojuanlu astrojuanlu force-pushed the refactor/remove-deprecatedenummeta branch 2 times, most recently from 9a08025 to 1064328 Compare June 28, 2023 06:24
@astrojuanlu astrojuanlu mentioned this pull request Jun 28, 2023
5 tasks
astrojuanlu and others added 4 commits June 28, 2023 12:02
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
@astrojuanlu astrojuanlu force-pushed the refactor/remove-deprecatedenummeta branch from 1064328 to 17c7e2f Compare June 28, 2023 10:02
@astrojuanlu astrojuanlu enabled auto-merge (squash) June 28, 2023 10:39
@astrojuanlu astrojuanlu merged commit c0d246f into main Jun 28, 2023
@astrojuanlu astrojuanlu deleted the refactor/remove-deprecatedenummeta branch June 28, 2023 10:47
@noklam noklam mentioned this pull request Jun 28, 2023
6 tasks
debugger24 pushed a commit to debugger24/kedro that referenced this pull request Jun 28, 2023
* Complete build requirements

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

* Add timeout for e2e process waiting

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

* Remove DeprecatedClassMeta in favor of getattr

Fix kedro-org/kedro-starters#137

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

* Ignore deprecation warnings in our own code

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

---------

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Co-authored-by: Deepyaman Datta <[email protected]>
Signed-off-by: debugger24 <[email protected]>
astrojuanlu added a commit that referenced this pull request Jun 28, 2023
* FIX: Typo in cli help

Signed-off-by: debugger24 <[email protected]>

* Make GitPod works (#2688)

* update base image

Signed-off-by: Nok <[email protected]>

* update gitpod image and apt-get

Signed-off-by: Nok <[email protected]>

* update docker

Signed-off-by: Nok <[email protected]>
---------

Signed-off-by: Nok <[email protected]>
Signed-off-by: debugger24 <[email protected]>

* Remove `DeprecatedClassMeta` in favor of `getattr` (#2724)

* Complete build requirements

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

* Add timeout for e2e process waiting

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

* Remove DeprecatedClassMeta in favor of getattr

Fix kedro-org/kedro-starters#137

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

* Ignore deprecation warnings in our own code

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

---------

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Co-authored-by: Deepyaman Datta <[email protected]>
Signed-off-by: debugger24 <[email protected]>

---------

Signed-off-by: debugger24 <[email protected]>
Signed-off-by: Nok <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Co-authored-by: Nok Lam Chan 陳諾林 <[email protected]>
Co-authored-by: Deepyaman Datta <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[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.

3 participants