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

Replace watchgod library with watchfiles #2134

Merged
merged 29 commits into from
Nov 4, 2024
Merged

Replace watchgod library with watchfiles #2134

merged 29 commits into from
Nov 4, 2024

Conversation

jitu5
Copy link
Contributor

@jitu5 jitu5 commented Oct 9, 2024

Description

Resolves #1589 and #2135

This pull request includes

Replacing the watchgod library with watchfiles.

watchfiles library updates its API with main method run_process and some other changes as well:

  • watcher_cls is removed and replaced by watch_filter which should be a simple callable, see filter docs
  • run_process method allow multiple paths to be watched, as result, the target argument to run_process & arun_process is now keyword-only

Please refer migration guild for more details.

Custom File Filter:

  • package/kedro_viz/autoreload_file_filter.py: Introduced AutoreloadFileFilter class to filter files based on allowed extensions and .gitignore patterns.
  • package/tests/test_autoreload_file_filter.py: Added tests for the new AutoreloadFileFilter class.

Development notes

Library Replacement and File Watching Mechanism Updates:

  • Replaced watchgod with watchfiles across various files (package/kedro_viz/launchers/cli/run.py, package/kedro_viz/launchers/jupyter.py, package/kedro_viz/server.py, package/tests/test_launchers/test_cli/test_run.py, package/tests/test_launchers/test_jupyter.py)

  • Add autoreload_file_filter.py class watch file filter package/tests/test_autoreload_file_filter.py

Dependency Updates:

  • Updated dependencies in requirements files to replace watchgod with watchfiles (package/features/steps/lower_requirements.txt, package/requirements.txt)

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

Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
@jitu5 jitu5 marked this pull request as ready for review October 9, 2024 19:40
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. thanks :)

@jitu5 jitu5 self-assigned this Oct 10, 2024
@ravi-kumar-pilla
Copy link
Contributor

Hi @jitu5 ,

This may not be related to watchgod -> watchfiles migration. But I am seeing more error logs when I edit files i.e.,

  1. Start in autoreload mode - kedro viz -a
  2. Start editing .py or any file that is being watched
  3. Save the changes and below error (similar errors) appear before rebuilding the app (this was raised earlier as well)
  4. Even without editing the file, if you just save the file, i.e., you are on .py file and do cmd+s -> raises error in the terminal

If possible, create a new ticket to have debounce for watching files and rebuilding the app or handle these errors someway for a cleaner watch experience. Since these errors have become frequent now, I would suggest we think of a solution before releasing this migration. Thank you

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/Ravi_Kumar_Pilla/opt/anaconda3/envs/kedro-viz-py39/lib/python3.9/multiprocessing/__init__.py", line 16, in <module>
    from . import context
  File "/Users/Ravi_Kumar_Pilla/opt/anaconda3/envs/kedro-viz-py39/lib/python3.9/multiprocessing/context.py", line 6, in <module>
    from . import reduction
  File "/Users/Ravi_Kumar_Pilla/opt/anaconda3/envs/kedro-viz-py39/lib/python3.9/multiprocessing/reduction.py", line 15, in <module>
    import pickle
  File "/Users/Ravi_Kumar_Pilla/opt/anaconda3/envs/kedro-viz-py39/lib/python3.9/pickle.py", line 43, in <module>
    from _pickle import PickleBuffer
KeyboardInterrupt

@jitu5
Copy link
Contributor Author

jitu5 commented Oct 11, 2024

Hi @jitu5 ,

This may not be related to watchgod -> watchfiles migration. But I am seeing more error logs when I edit files i.e.,

  1. Start in autoreload mode - kedro viz -a
  2. Start editing .py or any file that is being watched
  3. Save the changes and below error (similar errors) appear before rebuilding the app (this was raised earlier as well)
  4. Even without editing the file, if you just save the file, i.e., you are on .py file and do cmd+s -> raises error in the terminal

If possible, create a new ticket to have debounce for watching files and rebuilding the app or handle these errors someway for a cleaner watch experience. Since these errors have become frequent now, I would suggest we think of a solution before releasing this migration. Thank you

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/Ravi_Kumar_Pilla/opt/anaconda3/envs/kedro-viz-py39/lib/python3.9/multiprocessing/__init__.py", line 16, in <module>
    from . import context
  File "/Users/Ravi_Kumar_Pilla/opt/anaconda3/envs/kedro-viz-py39/lib/python3.9/multiprocessing/context.py", line 6, in <module>
    from . import reduction
  File "/Users/Ravi_Kumar_Pilla/opt/anaconda3/envs/kedro-viz-py39/lib/python3.9/multiprocessing/reduction.py", line 15, in <module>
    import pickle
  File "/Users/Ravi_Kumar_Pilla/opt/anaconda3/envs/kedro-viz-py39/lib/python3.9/pickle.py", line 43, in <module>
    from _pickle import PickleBuffer
KeyboardInterrupt

@ravi-kumar-pilla
I created a new virtual environment and followed the above steps to test but I didn't get any errors in logs. Could you try after crating new virtual environment. Also about Debouncing, run_process has debounce/step parameter, which we can use to introduce debounce for watcher.

@rashidakanchwala rashidakanchwala mentioned this pull request Oct 14, 2024
1 task
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
@Huongg
Copy link
Contributor

Huongg commented Oct 28, 2024

Hey @jitu5 after testing your PR, i got a similar error as @ravi-kumar-pilla has too

Screenshot 2024-10-28 at 10 21 37

@jitu5
Copy link
Contributor Author

jitu5 commented Oct 29, 2024

Hey @jitu5 after testing your PR, i got a similar error as @ravi-kumar-pilla has too

Screenshot 2024-10-28 at 10 21 37

@Huongg
The errors you shared are not the same as Ravi got, though they may be related to multiprocessing. Could you please share steps to reproduce the same error? Also have you created a new virtual environment to test ?

@Huongg
Copy link
Contributor

Huongg commented Oct 29, 2024

hey @jitu5 I tried a different environment and the error is clear now, it could have the previous environment. But I know have a different error but i might not be related to your changes numpy.core.multiarray failed to import

Signed-off-by: Jitendra Gundaniya <[email protected]>
@rashidakanchwala
Copy link
Contributor

hey @jitu5 I tried a different environment and the error is clear now, it could have the previous environment. But I know have a different error but i might not be related to your changes numpy.core.multiarray failed to import

not sure maybe you do pip install numpy again

Signed-off-by: Jitendra Gundaniya <[email protected]>
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.

Amazing! thanks @jitu5 !!

Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

I tested it again and its working fine for me, thanks @jitu5

@ravi-kumar-pilla
Copy link
Contributor

ravi-kumar-pilla commented Oct 30, 2024

Hi @jitu5 , The filter looks good but I see there are many reload requests. I saw we had this even before but is there a way to debounce these calls in watchfiles or a better way to handle file watching ?

I tested adding a change_test.json file under data/ folder which is ignored by git. Based on what I understood, the filter class should ignore the file change, instead it reloads Viz. Could you please confirm the behavior. Thank you

Signed-off-by: Jitendra Gundaniya <[email protected]>
@jitu5
Copy link
Contributor Author

jitu5 commented Oct 31, 2024

Hi @jitu5 , The filter looks good but I see there are many reload requests. I saw we had this even before but is there a way to debounce these calls in watchfiles or a better way to handle file watching ?

I tested adding a change_test.json file under data/ folder which is ignored by git. Based on what I understood, the filter class should ignore the file change, instead it reloads Viz. Could you please confirm the behavior. Thank you

@ravi-kumar-pilla
Yes change_test.json should be ignored. I tested the same scenario and it was reloading viz. But when I checked the .gitignore file of demo-project it has two line related to data folder.

# ignore everything in the following folders
data/**
# except their sub-folders
!data/**/

Because of the second line with except condition, pathspec is not interpreting .gitignore the same way as git does. But pathspec library has specialised class GitIgnoreSpec. Now I moved to GitIgnoreSpec and its ignoring change_test.json file under data/ folder correctly.

Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

Great work !! Thank you @jitu5

@jitu5 jitu5 merged commit 96cf45a into main Nov 4, 2024
25 checks passed
@jitu5 jitu5 deleted the feature/watchfiles branch November 4, 2024 11:52
@jitu5 jitu5 mentioned this pull request Nov 20, 2024
5 tasks
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.

watchgod package from requirements was renamed to watchfiles
4 participants