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

Refactor response classes #2113

Merged
merged 36 commits into from
Oct 30, 2024
Merged

Refactor response classes #2113

merged 36 commits into from
Oct 30, 2024

Conversation

ravi-kumar-pilla
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla commented Sep 25, 2024

Description

Partially resolves #2061

  • Module imports related to experiment tracking, strawberry graphQL (2-3sec) - We need to see if we can delay graphql call until someone clicks on ET, remove getVersions call (a middleware which ingests graphQL api app once a request for graphQL is made)
  • Separate api app (main api + additional api) - This will help remove dependencies involved for other apis
  • Module imports related to deployer factory (2-3sec) - Deferring deployer factory imports until a /deploy call is made
  • Separate responses module and try to get the main route working quickly - This needs refactoring and breaking down our responses module.
  • NetworkX takes time for import - Deferring the networkX import until used
  • SQLiteStore initialization also takes time (reason for configure and bootstrap project taking time) - Need to find a way to defer store initialization (likely a North star)
  • Apart from all the above improvements, uvicorn.run takes ~10sec to make the app available. Need to experiment by making a lighter app, reducing api routes and dependencies.

Development notes

  • Split responses.py file and group the response classes under responses folder which helps in scaling and maintaining the responses code base
  • Defer heavy imports like NetworkX, deployer factory imports until used
  • By experimenting found the real reason for uvicorn.run taking time is app building which involves importing all kedro project modules (example - for demo project, sklearn, matplotlib etc take time as shown below).
  • Updated tests and import references
image

Next steps:

QA notes

  • All tests should pass
  • Since the responses module is refactored, there needs to be manual testing for confirming the CLI and UI responses. It would be nice if we test kedro viz build, kedro viz deploy are intact
  • @jitu5 I think Kedro VSCode uses a function get_kedro_project_json_data in responses file which is moved to pipelines.py file. I would request you to have a look at this

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 new entries to the RELEASE.md file
  • Added tests to cover my changes

@ravi-kumar-pilla ravi-kumar-pilla changed the title Chore/refactor fast api Refactor response classes on Viz Oct 17, 2024
@ravi-kumar-pilla ravi-kumar-pilla changed the title Refactor response classes on Viz Refactor response classes Oct 18, 2024
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Hi, I had a think through this and came up with a few changes. Please let me know if it makes sense to you and happy to get on a call to discuss further.

  • save_responses.py file contains all the save_api and write_api functions. It's purpose it to handle saving API responses to FS.

  • utils. py file contains all encoding related functions classes + utility funcs (get_kedro_project_json)

  • node.py contains all response class for NodeAPI, and NodeMetadataAPI + get_node_metadata_response_function

  • pipeline.py contains all response classes for GraphAPI, NamedEntity, ModularPipelineResponses, and get_selected_pipeline_response, get_default_response function

  • metadata.py contains the same as you suggested.

  • base.py contains the BaseAPIResponse and APIErrorMesage response.

Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

LGTM!! :) thank you this is a big refactor. @ravi-kumar-pilla

Copy link
Contributor

@SajidAlamQB SajidAlamQB left a comment

Choose a reason for hiding this comment

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

Hey @ravi-kumar-pilla , thanks for this PR the refactoring looks good to me, I like the way you've split the files! I've left some minor comments!

package/kedro_viz/launchers/cli/utils.py Show resolved Hide resolved
package/kedro_viz/api/rest/responses/nodes.py Outdated Show resolved Hide resolved
@jitu5
Copy link
Contributor

jitu5 commented Oct 30, 2024

@ravi-kumar-pilla

@jitu5 I think Kedro VSCode uses a function get_kedro_project_json_data in responses file which is moved to pipelines.py file. I would request you to have a look at this

Are we going with pipelines.py or utils.py for get_kedro_project_json_data ?
To support for both Kdero-Viz version 10.0.0 and later in VSCode, I am thinking to add below

try:
    from kedro_viz.api.rest.responses.utils import get_kedro_project_json_data
except ImportError as e:
    from kedro_viz.api.rest.responses import get_kedro_project_json_data    

@ravi-kumar-pilla
Copy link
Contributor Author

Are we going with pipelines.py or utils.py for get_kedro_project_json_data ? To support for both Kdero-Viz version 10.0.0 and later in VSCode, I am thinking to add below

try:
    from kedro_viz.api.rest.responses.utils import get_kedro_project_json_data
except ImportError as e:
    from kedro_viz.api.rest.responses import get_kedro_project_json_data    

Hi @jitu5 , yes it will be in pipelines.py as it is now

@ravi-kumar-pilla ravi-kumar-pilla merged commit 31b0492 into main Oct 30, 2024
33 checks passed
@ravi-kumar-pilla ravi-kumar-pilla deleted the chore/refactor-fast-api branch October 30, 2024 19:33
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.

Refactor Kedro-Viz FAST API app creation
4 participants