pyronear / pyro-api Goto Github PK
View Code? Open in Web Editor NEWAlert Management API for wildfire prevention, detection & monitoring
License: Apache License 2.0
Alert Management API for wildfire prevention, detection & monitoring
License: Apache License 2.0
Following up on pyronear/pyro-platform#19, the following features need to be added to the client:
Considering the scopes of several routes are going to change (more restricted) with #45, this is all the more important since, if they are able to use GET on some routes without authentication, this might not be true in the future!
This is important information to have a comprehensive understanding of the visual situation of a device raising an alert.
Using this information + visual data + device orientation (yaw), the device can compute the azimuth of the alert/wildfire.
Hey there 👋
Playing around with the PRs, I noticed that some routes are protected by scopes while others aren't. If some of those are correct, I just want to make sure we do consider carefully those choices for each router:
Those where I have no clue whether we should add some:
cc @florianriche @jeanpasquier75 @fe51 @martin1tab
Hey there,
I ended up discovering that we can add a summary and description using the route decorator + its docstring (cf. https://fastapi.tiangolo.com/tutorial/path-operation-configuration/#description-from-docstring). I believe it would be a good idea to document each route (one PR for each separate router file in https://github.com/pyronear/pyro-api/tree/master/src/app/api/routes for easier reviewing):
Suggestions for documentation improvements are welcome!
Currently the app is hosted freely but with high constraints on up time, this needs to change when users require constant access. A few things need to be taken care of:
Hello everyone,
Here is a non-urgent issue related to this one in the Pyro-Platform repository!
In order to display past fires on the map, we are currently reading a local file stored within the repository. The goal would be to remove it and instead fetch this information via the Pyro-API. If I understood well, for now, there is no table dedicated to past fires (which could be useful not only for the platform but also for the data science team typically) and this issue is designed to discuss its addition to the database.
To be more precise about the historic_fires.csv
file, it was built out of a raw dataset of satellite fire detections which we preprocessed, notably in this Colab. We added several fields like the department and the "commune" where the fire occurred (as well as the population, area and density of the locality).
We wanted to use the latter to compute the density of population in the area, so as to approximately filter urban fires out of the dataset. However, we have not yet implemented this filtering on the platform and for now, department
is the only added field which is really required. I mention this in case we are limited for the number of fields.
Last remark: at some point, I guess we might want to add fires detected by our own devices and acknowledged by firemen to the past fires table in the database.
All that being said, here are my question(s):
historic_fires.csv
file as is to the database?Thanks a lot for your help, I hope that everything is clear!
Our data are supposed to be stored as UTC datetime
However, this information is implicit and we have no constraint on it.
What do you think of having datetime stored with time zone information ?
As an example, this format could be convenient "2020-04-01T12:34:56.000+0100".
That being said,to facilitate the use of our api, would be better to store, or at least, return all datetime in "UTC" (ie +0000). Thus, even if one user, get some that that could have been uploaded with differnt timezone, its gets all data with the same tz info.
So, once we agree on the following points, we can open PR and work on it :)
defining the datetime format (maybe we could use timestamptz datatype from postgreSQL ?, but I am not sure what is the easly compliant with sql alchemy)
deciding if we store all datetime with the same time zone information (always in UTC, ie +0000)
returning all datetime with the same time zone info (always in UTC, is +0000) (one day, we might add if needed an option to get data in a specific TZ or to infer the expected tz from the location .. )
Looking for feedbacks :)
Hey there,
As suggested in pyronear/pyro-platform#19, the platform will need a way to verify the login of its users. We suggested earlier to do this through the API, but I'm not certain about the best way to do this?
Should we create a route in accesses, that would only verify someone's credentials? Or should we have them create a token and then use that for other interactions with the API?
Add boolean field "is_relevant" to installation table.
In the case of a device that points a no_alert site it is needed to flag it with is_relevant as false in order to not consider alerts coming from this device.
If the related PR pass after alembic migration (#112), this feature need to be implemented as an alembic migration isn't ?
For now, the media that are uploaded to the bucket are stored indefinitely. But in the long-term, we need to erase those to avoid filling up the disk.
If that's alright with everyone, I suggest setting a MEDIA_PERSISTENCY value in the config (default 30-60days).
What do you think?
cc @jeanpasquier75 @florianriche @fe51
Add script to create super user
After a possible deletion of the volume of the db, it would be convenient to have a script to create a super user.
We might add a new folder called scripts with a first one, adding a super user with admin scope.
We have no route, for now, to specify an alert has been acknowledged and no way for a user to say an alert is complete.
The routes to do that should be quite straightforward.
One should also add functions to access those routes in the client.
Side note:
It also raises the question of how to interact with the events? I am not sure I clearly understand what they are supposed to do.
Currently, once the user is created by the admin, that person won't be able to update the password.
A specific route should be created to tackle this!
I was writing some unittests and apparently, the context we use for hashing seems to include a timestamp or something similar. As per https://github.com/pyronear/pyro-api/blob/master/src/app/api/security.py, here is how you can reproduce the error:
from passlib.context import CryptContext
pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto")
prev_hash = pwd_context.hash('hello')
print(prev_hash == pwd_context.hash('hello'))
which yields
False
I might be missing something here though
When one upload an image linked to a media (route upload_media) ; we:
1: Generate the CSP bucket key
2: Upload the image
3: If successful, alter the bucket key field in the DB
If one calls this function two times for a unique media, then the first uploaded image will stay in the CSP and we will loose its "key". It won't be linked to anything anymore. If for some reason, we do this too often there will be a load on the bucket storage.
Upon access creation, there is a verification to avoid login duplication in the table. However when using the corresponding put method, if the user changes her/his login to another existing one, nothing will check for duplication.
Following up on #45, I think it is sound to have a discussion about the multiple access levels in the API. With the current design of scopes, we cannot efficiently separate accesses of large user groups.
Simply put, I believe two fields are required:
groups
table with group_id
, and group_name
)Let's discuss here how we should create events, and link alerts to it.
device_id
The current media upload only updates the key/path of the media, while it does not upload content for the moment.
Qarnot computing support should be added as a supported bucket service.
Here is the documentation for it: https://computing.qarnot.com/documentation/sdk-python/api/storage/storage.html
Add example values to schemas that are used as input (see https://fastapi.tiangolo.com/tutorial/schema-extra-example/
This will speed up the testing when using the Swagger UI
no_alert
as SiteType in Site tableSome sites can be a source of smoke (like a quarry or a plant) and it is needed to record those specific location in order to not raise useless alerts. That could be done by adding a new site type "no_alert".
Moreover, users without admin scope should be able to create this new type of site. Hence a new route is needed !
Any feedback is welcome !
Have a convenient solution available to communicate from site
For a site equipped with the Pyronear solution, we are going to have several devices with camera, and one central device performing the inference (connected on a local network). So, only one device, the one doing inference will be exposed to the internet (for security aspects, in order to avoir entry points into our system, and to limit the number of actions to be carried out on devices with camera which will be exposed to sun and subject to quite high level of temperature)
Thus, only this one with communicate through the api and we need to be able to handle communication smoothly (that is to say, to be able to correctly route alerts from various devices with only one device using the api client).
2 possible solutions seem feasible :
The central device has credentials of all other devices, hence, via the api client, creation of one class per camera devices and authenticate for each camera, then it should be possible to manage classes and properly route communication.
It's a bit DIY though, and not very practical as the api is supposed to be able to facilitate the communication of devices.
Some evolution are performed on the api, in order to manager a group of devices on a site, with only one connected to the internet, and with a unique authentification.
What do you think @florianriche @jeanpasquier75 @frgfm ? There might be other ways to solve this issue.
I will be happy to discuss this, and organizing a quick meeting if necessary. Do not hesitate if you want me to clarify the issue !
In #13 a small issue was introduced, that wasn't caught up by unittests.
Fortunately, @jeanpasquier75 noticed it and it's being solved in #16. But this raises the question about how we should design our unittests
Any thoughts @florianriche @jeanpasquier75 @fe51 ?
There is no migration process with FastAPI or SQLAlchemy by default, we need to investigate our options here.
Initially mentioned in #17, an end-to-end testing script would allow us to easily make sure that the API can perform all required tasks for a specific scope. Several "interaction maps" could be designed, but here is a suggestion for a basic one:
What do you think?
As per pyronear/pyro-platform#19, for easy mapping and filtering, the sites table should hold two extra fields:
Having refactored most of the unittests, I came to the conclusion that the "me" scope is useless.
Simply put, here is a route using it now:
@router.get("/me", response_model=UserRead, summary="Get information about the current user")
async def get_my_user(me: UserRead = Security(get_current_user, scopes=["admin", "me"])):
"""
Retrieves information about the current user
"""
return me
Now if we use the user
scope instead:
@router.get("/me", response_model=UserRead, summary="Get information about the current user")
async def get_my_user(me: UserRead = Security(get_current_user, scopes=["admin", "user"])):
"""
Retrieves information about the current user
"""
return me
assuming all entries in the user table with "me" will be replaced by "user", we get the exact same behaviour.
There is no difference in "access level" between a "user" and "me" (or "device" and "me" depending on the situation). So I suggest that we remove it to avoid unnecessary complication. We will have to discuss access levels later on to distinguish a bare user from a "supervisor" or a "manager".
I suggest waiting for #138 approval before starting a PR to avoid conflicts.
As per pyronear/pyro-platform#19 (comment), we need to able to tell how many devices (list them would avoid creating another route later) are on a specific site at a given time.
Addind a route /list-devices
in installations would be the best option in my opinion
From Mateo's request:
En gros j'aimerais qu'on mette en place une petite présentation ou je filme un départ de feu et je ping l'api quand j'en détecte un et du coup ca vous permet d'enchaîner sur la présentation de l'api. Je dois utiliser quoi pour ca ?
It seems we lack the proper client's routes (and potentially API routes) to achieve that.
The way a client is instanced right now is buggy (guilty here) because everytime we create a client instance, we alter the Class variable routes.
As a consequence, if we want to create two client instances (for whatever reason), the second instance will have incorrect routes as it will use the already altered class variable.
class Client:
routes = {"token": "/login/access-token",
...
}
def __init__(self, api_url, credentials_login, credentials_password):
self.api = api_url
self._add_api_url_to_routes()
...
def _add_api_url_to_routes(self):
for k, v in self.routes.items():
self.routes[k] = urljoin(self.api, v)
...
Suggested solutions:
Currently, the requirements file does have some loose version constraints on a few libraries, namely python-jose
and passlib
. Perhaps we should specify those a bit further?
How to:
Need to coordinate with all teams
While I was changing scopes on some routes, I noticed something:
get_current_access
yields an access with insufficient scope, the request is still being processedWe would need to investigate and fix this to be able to check scopes in the unittests.
Hello there 👋
Following up on pyronear/pyro-platform#27, perhaps we should add right away the possibility of acknowledging an alert in the client. I think we will need to think further later about which scope/access should be required to used it, but for now we can restrict it to admin?
What do you think @florianriche @jeanpasquier75 @fe51 @martin1tab ?
Currently, only admins can register devices, while a user should be able to register her/his own device.
A draft PR #20 has been opened to upload a media received by the API on S3.
Let's discuss the pros's and con's of the different solutions and the following topics.
TODO:
Currently, we use plain HTTP status code to raise exceptions, while there are many existing status codes that could be useful in some cases. If the API is to be used widely for system integration, I think we need to consider to specify those.
Here is a reference we could use: https://kinsta.com/blog/http-status-codes/
& the status code used by FastAPI (from starlette): https://github.com/encode/starlette/blob/master/starlette/status.py
What do you think?
Following up on #112 (more specifically #112 (comment)), we need to automate the alembic migration whenever it needs to be run. Depending on the integration, Github Workflow could help with that!
Add azimut
field to alerts table
In order to create a vision cone on the platform, it is necessary to know the azimut of an alert in relation withthe device that raised it.
Once we are able to identify the GPS position of an alert, we can extrapolate the azimut from the GPS position of the alert and the one from the device
We need to develop on Client-side (Device) in order to send information to the API.
The devices will need to:
Now comes the question: how to develop that client-side capability?
While designing new unittests, I figured I wasn't sure about what to expect when we use query_filters with multiple filters on a single value. Since this does not include order relationships but strictly equality, I suggest changing this to a dictionary to prevent duplication of keys.
Here is an example on the fetch_all crud function:
what a call currently looks like
result_entries = crud.fetch_all(user_table, [('username', 'fg')]
my suggestion
result_entries = crud.fetch_all(user_table, {'username': 'fg'}]
I think this would imply:
post_access
in https://github.com/pyronear/pyro-api/blob/master/src/app/api/routes/accesses.py#L13create_access_token
(https://github.com/pyronear/pyro-api/blob/master/src/app/api/routes/login.py#L16)Currently, when requesting an image through the client:
So we at least get a double latency on content transfer. A more optimal solution would be:
We could expect ~50% drop in latency in doing so!
Even after #47 , their was still a bug on deployment. Here are the logs:
2020-11-22T07:58:34.305128+00:00 app[web.1]: File "/app/.heroku/python/lib/python3.6/site-packages/starlette/routing.py", line 526, in lifespan
2020-11-22T07:58:34.305129+00:00 app[web.1]: async for item in self.lifespan_context(app):
2020-11-22T07:58:34.305129+00:00 app[web.1]: File "/app/.heroku/python/lib/python3.6/site-packages/starlette/routing.py", line 467, in default_lifespan
2020-11-22T07:58:34.305131+00:00 app[web.1]: await self.startup()
2020-11-22T07:58:34.305131+00:00 app[web.1]: File "/app/.heroku/python/lib/python3.6/site-packages/starlette/routing.py", line 502, in startup
2020-11-22T07:58:34.305132+00:00 app[web.1]: await handler()
2020-11-22T07:58:34.305132+00:00 app[web.1]: File "src/app/main.py", line 18, in startup
2020-11-22T07:58:34.305133+00:00 app[web.1]: await init_db()
2020-11-22T07:58:34.305133+00:00 app[web.1]: File "src/app/db/init_db.py", line 16, in init_db
2020-11-22T07:58:34.305134+00:00 app[web.1]: hashed_password = await hash_password(cfg.SUPERUSER_PWD)
2020-11-22T07:58:34.305134+00:00 app[web.1]: File "src/app/api/security.py", line 31, in hash_password
2020-11-22T07:58:34.305134+00:00 app[web.1]: return pwd_context.hash(password)
2020-11-22T07:58:34.305135+00:00 app[web.1]: File "/app/.heroku/python/lib/python3.6/site-packages/passlib/context.py", line 2258, in hash
2020-11-22T07:58:34.305135+00:00 app[web.1]: return record.hash(secret, **kwds)
2020-11-22T07:58:34.305136+00:00 app[web.1]: File "/app/.heroku/python/lib/python3.6/site-packages/passlib/utils/handlers.py", line 777, in hash
2020-11-22T07:58:34.305136+00:00 app[web.1]: validate_secret(secret)
2020-11-22T07:58:34.305137+00:00 app[web.1]: File "/app/.heroku/python/lib/python3.6/site-packages/passlib/utils/handlers.py", line 122, in validate_secret
2020-11-22T07:58:34.305190+00:00 app[web.1]: raise exc.ExpectedStringError(secret, "secret")
2020-11-22T07:58:34.305190+00:00 app[web.1]: TypeError: secret must be unicode or bytes, not None
2020-11-22T07:58:34.305191+00:00 app[web.1]:
2020-11-22T07:58:34.305283+00:00 app[web.1]: ERROR: Application startup failed. Exiting.
It was related to https://github.com/pyronear/pyro-api/blob/master/src/app/config.py#L24-L25, so I updated the environment variables on the deployment server and it's OK now.
We might need to raise an error if the environment variables are not set!
In order to send notifications on multiple systems based on alert raising on the API-side, we need to provide developers with a webhook option. Another front-end service of pyronear is already in need for this feature cf. pyronear/pyro-platform#38
Fortunately, FastAPI callbacks seem to be quite appropriate for this use case. However, I'm not exactly sure how to make a webhook available using that.
[] Add table to record metrics (cpu and memory usage, temperature) from pi devices
[] Create related routes
[] Update client consequently
In order to design a suitable system, some metrics should be recorded.
After test phases, the output can be analyzed to understand how to improve the system or understand why it crashed.
As an example, in order to know if fans, heatsink are needed, cpu temperature can be recorded; Or, some performance about cpu usage or memory available could help to select the best raspberry pi model fitting our needs.
Moreover, it will help us to monitor systems.
Hence, a new table needs to be created, and could look like this :
device_id | datetime | measure_name | value | units |
---|
It will be time related metrics having only relation with devices.
Hence, for sake of simplicity, it can be stored in our postgre table (Otherwise it would implies a specific db system, what do you think)
Granularity of metrics might be around every 10min
This is completely open to discussion about and all suggestion are welcomed
Currently when a PUT request is done and one optional field is not specified it is updated to null.
A solution should be to add an option boolean to crud.put or crud.update_entry, if set to True it will update only the not-None fields
The current state of the client documentation is not really helpful. Most of the payloads are not detailed in the docstrings and thus in the documentation.
This is the documentation for the get_site_devices
method:
Worst thing about this: in this case, the payload is really simple (site_id). I think we should do something about that soon 😅
Currently, the API supports the update of location information for a given device provided admin scope. However, a device with GPS capabilities should be able to update its own location.
This might be a good occasion to add a route to update the location of the current_device!
In order to avoid requiring ssh access to our devices, it would be handy for them to check whether they need to update and automatically do it if necessary.
I gave it some thoughts and here is what I suggest:
specs
field (which we should rename to hardware_specs
), we introduce a software_hash
field.software_hash
. If it's different from the one it uses, then it updates.I strongly suggest going for the commit hash, at least to check the mechanism
What do you think @pyronear/back-end ?
When we first designed all the table fields, we added the device_id
as the emitter of the alert. I'm starting to think it would make way more sense to replace this by installation_id
. There are pros and cons
Here is how the change would play out:
This would mean that a device per se cannot send an alert without being an installation itself i.e. linked to a site (the biggest con). The API client will handle the alert sending by resolving the installation from the device. Additionally:
Before you were receiving the device_id:
Now you would receive the installation_id:
One last option would be to have both but I feel like this would be a dangerous path later on. What do you think @pyronear/back-end ?
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.