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

Eliminate globals usage #2790

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Eliminate globals usage #2790

wants to merge 2 commits into from

Conversation

kruton
Copy link
Contributor

@kruton kruton commented Nov 19, 2024

This will help running tests because the app isn't initialized automatically by touching the "changedetectionio" package. Moving things out of the __init__.py removes the side-effect of "import changedetection" which means tests can control the state without restarting.

This is the first step in making the tests run with only calling "pytest". The fixture use and test setup need to be adjusted to not depend on test ordering.

@kruton kruton force-pushed the no-globals branch 4 times, most recently from bb39faa to ee417ba Compare November 19, 2024 07:10
requirements.txt Outdated Show resolved Hide resolved
@dgtlmoon
Copy link
Owner

Would be awesome if you can add this on another PR and base it on this branch/PR #2760

eventlet @ git+https://github.com/eventlet/eventlet@4d48d10b990910cc87ec6bbeeaea63b295fe314d

@dgtlmoon
Copy link
Owner

thanks again - tests are being a bit flakey, i'll keep pushing and see whats up

@dgtlmoon
Copy link
Owner

could this run the basic tests in parallel then too? (faster)

@dgtlmoon
Copy link
Owner

The fixture use and test setup need to be adjusted to not depend on test ordering.

can you branch from this branch and also do that work on another PR?

@kruton
Copy link
Contributor Author

kruton commented Nov 19, 2024

The fixture use and test setup need to be adjusted to not depend on test ordering.

can you branch from this branch and also do that work on another PR?

Yes, it will take a while because there are many tests and I don't quite understand the test fixture setup yet.

I actually have another branch in my local repo where I changed the entire codebase to use Quart, but the change is gigantic. Doing these smaller changes first will help make the transition to asyncio more smooth if you want to go that direction.

@kruton
Copy link
Contributor Author

kruton commented Nov 19, 2024

could this run the basic tests in parallel then too? (faster)

It should be able to using pytest-xdist.

@kruton kruton force-pushed the no-globals branch 3 times, most recently from f396b13 to 266b879 Compare November 19, 2024 17:34
@dgtlmoon
Copy link
Owner

The fixture use and test setup need to be adjusted to not depend on test ordering.

can you branch from this branch and also do that work on another PR?

Yes, it will take a while because there are many tests and I don't quite understand the test fixture setup yet.

I actually have another branch in my local repo where I changed the entire codebase to use Quart, but the change is gigantic. Doing these smaller changes first will help make the transition to asyncio more smooth if you want to go that direction.

amazing :) first question, what is Quart?

'It should be able to using pytest-xdist." that would be neet, also if the run-basic-test.sh was updated

@dgtlmoon
Copy link
Owner

Doing these smaller changes first will help make the transition to asyncio more smooth if you want to go that direction.

problem is the architecture of the app, it's basically just a single threaded app because it has this kind of global dictionary of data "datastore" that cant be accessed from different threads if the flask wrapper launched a new thread per request

which is why i guess mysql/couchdb/etc exist - tho i initially started this app in the fastest easiest way i could without worrying about huge database setups, i really like the flat JSON way

I would really prefer to have a "settings.json" in each "datastore" watch UUID directory rather than in one huge url-watches.json tho, i tried to deal with that earlier

@kruton
Copy link
Contributor Author

kruton commented Nov 19, 2024

Doing these smaller changes first will help make the transition to asyncio more smooth if you want to go that direction.

problem is the architecture of the app, it's basically just a single threaded app because it has this kind of global dictionary of data "datastore" that cant be accessed from different threads if the flask wrapper launched a new thread per request

which is why i guess mysql/couchdb/etc exist - tho i initially started this app in the fastest easiest way i could without worrying about huge database setups, i really like the flat JSON way

I would really prefer to have a "settings.json" in each "datastore" watch UUID directory rather than in one huge url-watches.json tho, i tried to deal with that earlier

Yes, I noticed that JSON files were preferred from looking at the code.

Quart is Flask but using asyncio. So everything is run in a single thread but it yields when it will do something that blocks the processor. In my quart branch, I've removed all the threads and used asyncio for everything.

For instance, when sync_to_json is called, it uses aiofiles to write the JSON database to make sure that the file operation doesn't block the other tasks in the asyncio event loop. Files are actually written in a subprocess, but that detail is hidden from your view.

@dgtlmoon
Copy link
Owner

Quart looks amazing, because i'de also like to add some kind of websocket for better live updates too

@@ -327,7 +316,7 @@ def login():

password = request.form.get('password')

if (user.check_password(password)):
if (user.check_password(password, datastore=datastore)):
Copy link
Owner

Choose a reason for hiding this comment

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

why cant check_password access the "g" ? it could be called g.datastore ? then it would mean i can stop passing "datastore" around everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it could be done that way. I don't like putting globals in because it makes testing problematic. I don't even know how using g works with tests.

Copy link
Owner

Choose a reason for hiding this comment

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

I'll admit, i dont know much about this :) just an idea, yes, agreed

This will help running tests because the app isn't initialized
automatically by touching the "changedetectionio" package. Moving things
out of the __init__.py removes the side-effect of "import
changedetection" which means tests can control the state without
restarting.

This is the first step in making the tests run with only calling
"pytest". The fixture use and test setup need to be adjusted to not
depend on test ordering.
This should make the code a little cleaner to use these proxy objects.
@dgtlmoon
Copy link
Owner

heya, whats your thoughts on this PR? as in, how much more work should go into it? what else are you hoping to solve here?

@dgtlmoon
Copy link
Owner

if functions like this in Watch could access g globals for getting default values of the datastore settings, that would be AMAZING, then I can throw away a lot of parsing and special switch code across the whole application

def get_fetch_backend(self):
"""
Like just using the `fetch_backend` key but there could be some logic
:return:
"""
# Maybe also if is_image etc?
# This is because chrome/playwright wont render the PDF in the browser and we will just fetch it and use pdf2html to see the text.
if self.is_pdf:
return 'html_requests'
return self.get('fetch_backend')

is it possible? Because "Watch" object cant "see" the global datastore values in the case that it needs to default to some value if the 'watch' is to default/null etc

@dgtlmoon
Copy link
Owner

dgtlmoon commented Dec 4, 2024

Hello, this is a very long PR and it needs to be resolved ASAP or it will get more and more conflicts and probably get closed/lost - i would really love to resolve this

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.

2 participants