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

Deferred dependencies import into run_server #2060

Merged
merged 4 commits into from
Sep 3, 2024

Conversation

jitu5
Copy link
Contributor

@jitu5 jitu5 commented Aug 28, 2024

Description

Resolves kedro-org/vscode-kedro#72

#2049 To get current Kedro project JSON data required by Kedro-Viz React component when running inside VSCode, We introduce get_kedro_project_json_data function in responses.py which returns current Kedro project JSON data. But before get_kedro_project_json_data we need to load and populate data with load_and_populate_data method from server.py

To avoid ImportError when importing load_and_populate_data from server.py in VSCode extension, Deferred dependencies like fsspec, uvicorn, multiprocessing and watchgod inside Run_server.

Development notes

  • Moved dependencies like fsspec, uvicorn, multiprocessing and watchgod inside Run_server.

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

jitu5 added 4 commits August 23, 2024 14:56
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@jitu5 jitu5 self-assigned this Aug 28, 2024
@jitu5 jitu5 requested review from ravi-kumar-pilla and rashidakanchwala and removed request for rashidakanchwala and ravi-kumar-pilla August 28, 2024 15:49
@jitu5 jitu5 changed the title Deferred dependencies import for Run_server Deferred dependencies import into run_server Aug 28, 2024
@jitu5 jitu5 requested a review from noklam August 30, 2024 15:39
@noklam
Copy link
Contributor

noklam commented Aug 30, 2024

Is it possible to extract load and populate data instead? That doesn't sound like something belong to server.py to start with.

@jitu5
Copy link
Contributor Author

jitu5 commented Aug 30, 2024

Is it possible to extract load and populate data instead? That doesn't sound like something belong to server.py to start with.

@noklam I started with that moved populate_data and load_and_populate_data to separate file load_data.py but @ravi-kumar-pilla suggested to use deferred dependencies approach similar to what we did here #1920 (comment)

@ravi-kumar-pilla
Copy link
Contributor

Is it possible to extract load and populate data instead? That doesn't sound like something belong to server.py to start with.

I think we can definitely move load data to a different file.

@noklam I started with that moved populate_data and load_and_populate_data to separate file load_data.py but @ravi-kumar-pilla suggested to use deferred dependencies approach similar to what we did here #1920 (comment)

I might be missing some context here. I was in an impression that you created the file just to resolve the import errors. So suggested to move the imports within the function. We can do the refactoring as well.

@noklam
Copy link
Contributor

noklam commented Aug 30, 2024

I started with that moved populate_data and load_and_populate_data to separate file load_data.p

What would that look likes?

@ravi-kumar-pilla suggested to use deferred dependencies approach similar to what we did here #1920 (comment)

What's the reason for that? I am fine with the deferred approach but I think load_data, load_and_populate_data does not belongs to server.py. There are some rooms for de-coupling as well, as

, include_hook also doesn't affect the data at all. This bundle the data for flowchart and the application itself (i.e. dataset preview).

@jitu5
Copy link
Contributor Author

jitu5 commented Aug 30, 2024

Is it possible to extract load and populate data instead? That doesn't sound like something belong to server.py to start with.

I think we can definitely move load data to a different file.

@noklam I started with that moved populate_data and load_and_populate_data to separate file load_data.py but @ravi-kumar-pilla suggested to use deferred dependencies approach similar to what we did here #1920 (comment)

I might be missing some context here. I was in an impression that you created the file just to resolve the import errors. So suggested to move the imports within the function. We can do the refactoring as well.

Yes, I created the file just to resolve the import errors.

@jitu5
Copy link
Contributor Author

jitu5 commented Aug 30, 2024

I started with that moved populate_data and load_and_populate_data to separate file load_data.p

What would that look likes?

80d35b6#diff-94c1c8a57783f10e120cb066fe8582a44e256679802723204e8b5b4d7fd8d6b4

@ravi-kumar-pilla
Copy link
Contributor

What's the reason for that? I am fine with the deferred approach but I think load_data, load_and_populate_data does not belongs to server.py. There are some rooms for de-coupling as well, as

I agree with you, there is definitely a room to refactor the server.py file and keep it only to run server. We do have a refactoring ticket here - #2061 and we can consider some refactor of server.py in this.

, include_hook also doesn't affect the data at all. This bundle the data for flowchart and the application itself (i.e. dataset preview).

include_hook is used to create a NullPluginManager when we do KedroSession._hook_manager. Again, we can decouple and create more pure python functions.

We can go ahead with deferred imports for now if it makes sense and you can add suggestion to #2061 or we can create a different ticket to refactor these as it might involve file changes in tests as well and other references and keep PRs small. Let me know what do you think

Thank you

@noklam
Copy link
Contributor

noklam commented Aug 30, 2024

Perfect, since there is a refactor ticket going on let's do the proper refactoring there. For now keeping this import deferred is sufficient.

@noklam noklam mentioned this pull request Sep 2, 2024
21 tasks
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

As discussed in the comment, let's do this as deferred import and address the refactoring in a separate PR.

@jitu5 jitu5 merged commit 88829c8 into feature/kedro-project-data Sep 3, 2024
34 checks passed
@jitu5 jitu5 deleted the vsc/load_data branch September 3, 2024 09:29
jitu5 added a commit that referenced this pull request Sep 3, 2024
…on (#2049)

* get_kedro_project_json_data function added

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>

* lint fix

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>

* Lint fix

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>

* Test fix

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>

* lint fix

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>

* Deferred dependencies import into run_server (#2060)

* Load data functions move out of server

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>

* Refactor

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>

* Lint fix

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>

* lint fix

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>

---------

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>

---------

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Co-authored-by: Nok Lam Chan <nok_lam_chan@mckinsey.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.

4 participants