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

Improve CI on Kedro-Viz #1805

Open
1 task done
ravi-kumar-pilla opened this issue Mar 15, 2024 · 7 comments · Fixed by #2204
Open
1 task done

Improve CI on Kedro-Viz #1805

ravi-kumar-pilla opened this issue Mar 15, 2024 · 7 comments · Fixed by #2204
Assignees
Labels
Enhancement Python Pull requests that update Python code

Comments

@ravi-kumar-pilla
Copy link
Contributor

Description

Improve CI execution time for backend unit tests which should not rely on building the react application, rather depend only on backend functions.

Context

When we are running unit_tests on CircleCI, during build_backend we have the following steps -

unit_tests:
    parameters:
      python_version:
        type: string
    executor:
      name: docker
      python_version: <<parameters.python_version>>
    steps:
      - setup
      - run:
          name: Run Python tests
          command: make pytest

The setup step has -

  setup:
    steps:
      - checkout
      - setup_python_env
      - install_node_dependencies
      - npm_build
  • Apart from Installing Python dependencies which takes (>3m) , Build React application takes (>2m).
  • We need to refactor our unit tests if needed to test only python functions and do not depend on building react application
  • Building the react app should only be part of frontend tests and e2e tests.

Possible Implementation

  • Remove all frontend build steps (Install Node.js -> Build React application)
  • Refactor pytests to rely only on python env and backend functions

Possible Alternatives

Checklist

  • Include labels so that we can categorise your feature request
@ravi-kumar-pilla ravi-kumar-pilla added Enhancement Python Pull requests that update Python code labels Mar 15, 2024
@ravi-kumar-pilla ravi-kumar-pilla moved this to Inbox in Kedro-Viz Mar 15, 2024
@NeroOkwa NeroOkwa moved this from Inbox to Backlog in Kedro-Viz Mar 25, 2024
@rashidakanchwala
Copy link
Contributor

rashidakanchwala commented Nov 20, 2024

@astrojuanlu , @ravi-kumar-pilla - I can't find the other ticket bur our CI takes very very long for each commit. I remember we had done improvements last time that got us from 30 min to 10 min, and now it's back to 30+ minutes. This really slows us down. Can we reprioritise this ticket. Maybe find dev time to work on it in December? , since there's no major feature work happening as a lot of us are on break.

@astrojuanlu
Copy link
Member

Looking at https://github.com/kedro-org/kedro-viz/actions/runs/11933469103, looks like

  • e2e_tests: ~25 minutes
  • lint: ~15 minutes
  • unit_tests: ~18 minutes
e2e_tests lint unit_tests
image image image

I agree @rashidakanchwala, let's tackle this ASAP! As early as right now if we have the resources.

@astrojuanlu
Copy link
Member

astrojuanlu commented Nov 20, 2024

About lint, it takes 13 minutes just to install the dependencies and half a minute actually running the linter.

It's important to note that linting and formatting does not need the runtime dependencies. In other words: you can ruff format the codebase of Kedro Viz without installing the dependencies of Kedro Viz. mypy does need the dependencies installed though.

Unit tests also spend 17 minutes just setting things up, half a minute running the tests.

e2e spends 16 minutes just installing things.

I think it's very clear that uv can improve the performance.

@astrojuanlu astrojuanlu moved this from Backlog to Todo in Kedro-Viz Nov 20, 2024
@Huongg
Copy link
Contributor

Huongg commented Nov 26, 2024

This issue will be addressed in two phases:

  1. Enhance the runtime performance by using uv, which is being implemented in this PR.
  2. Refactor Back-End Unit Tests: The current back-end unit tests are overly dependent on the front-end, which should not be the case. This refactoring effort is estimated at 5–8 points. I’ll either open a new ticket for this or reuse the existing ticket originally created by Ravi.
    • Currently, there are seven failing tests that need to be restructured to remove their dependency on the front-end.
FAILED tests/test_api/test_apps.py::TestIndexEndpoint::test_index - FileNotFoundError: [Errno 2] No such file or directory: '/home/runner/work/kedro-viz/kedro-viz/package/kedro_viz/html/index.html'
FAILED tests/test_api/test_apps.py::TestIndexEndpoint::test_heap_enabled - FileNotFoundError: [Errno 2] No such file or directory: '/home/runner/work/kedro-viz/kedro-viz/package/kedro_viz/html/index.html'
FAILED tests/test_api/test_apps.py::TestReloadEndpoint::test_autoreload_script_added_to_index - FileNotFoundError: [Errno 2] No such file or directory: '/home/runner/work/kedro-viz/kedro-viz/package/kedro_viz/html/index.html'
FAILED tests/test_api/test_apps.py::TestFaviconEndpoint::test_favicon_endpoint - RuntimeError: File at path /home/runner/work/kedro-viz/kedro-viz/package/kedro_viz/html/favicon.ico does not exist.
FAILED tests/test_api/test_rest/test_responses/test_pipelines.py::TestAPIAppFromFile::test_api_app_from_json_file_main_api - RuntimeError: Directory '/home/runner/work/kedro-viz/kedro-viz/package/kedro_viz/html/static' does not exist
FAILED tests/test_api/test_rest/test_responses/test_pipelines.py::TestAPIAppFromFile::test_api_app_from_json_file_index - RuntimeError: Directory '/home/runner/work/kedro-viz/kedro-viz/package/kedro_viz/html/static' does not exist
FAILED tests/test_integrations/test_base_deployer.py::TestBaseDeployer::test_upload_static_files - FileNotFoundError: [Errno 2] No such file or directory: '/home/runner/work/kedro-viz/kedro-viz/package/kedro_viz/html/index.html'

@Huongg
Copy link
Contributor

Huongg commented Nov 26, 2024

Additionally, in setup_tests, it would be ideal to avoid installing front-end dependencies unless running e2e tests. We could consider adding a condition similar to the example below to address this.

 - name: Setup Node.js and Install Dependencies
      if: github.workflow == 'e2e-tests.yml'
      uses: "./.github/actions/install_node_dependencies"

    - name: Build React application
      if: github.workflow == 'e2e-tests.yml'
      run: |-
        node -v
        make build
      shell: bash  

@github-project-automation github-project-automation bot moved this from In Review to Done in Kedro-Viz Nov 26, 2024
@Huongg Huongg reopened this Nov 26, 2024
@rashidakanchwala rashidakanchwala changed the title Improve Backend Unit test execution on CI Improve CI for Kedro-Viz Nov 26, 2024
@rashidakanchwala rashidakanchwala changed the title Improve CI for Kedro-Viz Improve CI on Kedro-Viz Nov 26, 2024
@astrojuanlu
Copy link
Member

What's left to do here? Also is this connected with #1908 ?

@ravi-kumar-pilla
Copy link
Contributor Author

What's left to do here? Also is this connected with #1908 ?

Both the tickets are on me :) ....

  • For the current ticket - we need to refactor backend pytests not to rely on frontend build i.e., steps - install_node_dependencies, npm_build should not be required for pytest
  • For Remove Github Action duplicate workflows #1908 - Since this was low priority, did not really start on it. But the idea is to eliminate duplicate workflows.

I can take these 2 tickets in the coming sprints once the notebook work is done. Thank you

cc: @rashidakanchwala

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Python Pull requests that update Python code
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants