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

Dev #7

Merged
merged 94 commits into from
Apr 17, 2023
Merged

Dev #7

merged 94 commits into from
Apr 17, 2023

Conversation

asiripanich and others added 24 commits December 19, 2022 16:55
Restructing the project requires the following fixes:
- fixed the docker build context.
- fixed the app's data path.
Other changes include:
- restructured the project.
The following elements have been added:
- sign-ups trend plot,
- trips trend plot,
- number of users box,
- number of trips box, and
- number of active users box.
@asiripanich asiripanich self-assigned this Jan 24, 2023
@shankari
Copy link
Contributor

@asiripanich should I start reviewing this? Or are you reviewing it yourself?

@AlirezaRa94
Copy link
Contributor

To change the STUDY_NAME, I have to make changes in most parts of the code, and I should test everything again. It takes some time, but I will try to finish it soon.

Why? you only read the environment variable in one location (permissions.py), so you should be able to only read the study config from the location in one place.

STUDY_NAME = os.getenv('STUDY_NAME')

It was an environment variable, but now I have to use a callback to get the STUDY_NAME from URL, and I don't have access to it in the permissions.py file.

@shankari
Copy link
Contributor

shankari commented Apr 5, 2023

It was an environment variable, but now I have to use a callback to get the STUDY_NAME from URL, and I don't have access to it in the permissions.py file.

There must be a way to transfer the data from the callback to permissions.py.

  • You can make a call to permissions.py to set the study name
  • you can set the environment variable in the callback
  • ...

@AlirezaRa94
Copy link
Contributor

It was an environment variable, but now I have to use a callback to get the STUDY_NAME from URL, and I don't have access to it in the permissions.py file.

There must be a way to transfer the data from the callback to permissions.py.

  • You can make a call to permissions.py to set the study name
  • you can set the environment variable in the callback
  • ...

Thank you, If you are OK with these methods, I will make changes using them.

@shankari
Copy link
Contributor

shankari commented Apr 5, 2023

Thank you, If you are OK with these methods, I will make changes using them.

The ideal way is to register the callback from permissions.py so that you don't have unnecessary dependencies
But if that is not possible, then the methods above are OK.
And are strictly better than sprinkling the location callback across multiple locations in the code, if that is what you wanted to do.

For completeness, if registering the callback from permissions.py is not possible, please document what you tried and why it didn't work.

@AlirezaRa94
Copy link
Contributor

Thank you, If you are OK with these methods, I will make changes using them.

The ideal way is to register the callback from permissions.py so that you don't have unnecessary dependencies But if that is not possible, then the methods above are OK. And are strictly better than sprinkling the location callback across multiple locations in the code, if that is what you wanted to do.

For completeness, if registering the callback from permissions.py is not possible, please document what you tried and why it didn't work.

We can create a callback inside permissions.py. However, the callback won't automatically run when the module is loaded because it is outside the application script. So we'll need to trigger it somehow inside the application script, which is not different from setting it outside the module.

@shankari
Copy link
Contributor

@AlirezaRa94 any updates? it's been a week since the last update and we need to deploy this...

@AlirezaRa94
Copy link
Contributor

@shankari I'm busy these days, but I'll try to find some free time to work on it this week.

@shankari
Copy link
Contributor

@AlirezaRa94 the goal was to have it fully done and ready to deploy by end of March. It is now mid-Apr, and we need to be showing off demos...

@AlirezaRa94
Copy link
Contributor

AlirezaRa94 commented Apr 13, 2023

@AlirezaRa94 the goal was to have it fully done and ready to deploy by end of March. It is now mid-Apr, and we need to be showing off demos...

The goal was to have the time-use feature and the dashboard done by the end of February. The dashboard with authentication and dynamic config was ready at that time. At that time, my task was working full-time on the dashboard. But now I have other duties and must find some free time to work on the dashboard.
Right now, I'm working on your comments which I got on 21 March. However, I've addressed most of your comments.
Removing the STUDY_NAME from the docker-compose file is not as simple as you think, which I have explained why here #7 (comment)

@AlirezaRa94
Copy link
Contributor

@shankari
Now that we have STUDY_NAME in the docker-compose, we have the dynamic config before loading the dashboard layouts. But if we want to infer it from the URL, we need to get it in a request, and this happens after loading the dashboard, which will break everything.
To solve this problem, we can add a welcome page and set the dynamic config at this time, and then we can load the dashboard.
I'm also open to hearing your ideas on this.

@shankari
Copy link
Contributor

shankari commented Apr 13, 2023

@AlirezaRa94

Right now, I'm working on your comments which I got on 21 March.

That's because the dynamic config was only done on 20th March (dbae8f9)

However, I've addressed most of your comments.

You do need to make the changes to use the timeseries functions instead of a db_util - we are getting an error with the queries you are using

File "/root/miniconda-4.12.0/envs/emission/lib/python3.7/site-packages/dash/_callback.py", line 447, in add_context
output_value = func(*func_args, **func_kwargs)  # %% callback invoked %%
File "app_sidebar_collapsible.py", line 190, in update_store_trips
df = query_confirmed_trips(start_date_obj, end_date_obj)
File "/usr/src/app/utils/db_utils.py", line 66, in query_confirmed_trips
df = pd.json_normalize(list(query_result))
File "/root/miniconda-4.12.0/envs/emission/lib/python3.7/site-packages/pymongo/cursor.py", line 1207, in next
if len(self.__data) or self._refresh():
File "/root/miniconda-4.12.0/envs/emission/lib/python3.7/site-packages/pymongo/cursor.py", line 1124, in _refresh
self.__send_message(q)
File "/root/miniconda-4.12.0/envs/emission/lib/python3.7/site-packages/pymongo/cursor.py", line 1001, in __send_message
address=self.__address)
File "/root/miniconda-4.12.0/envs/emission/lib/python3.7/site-packages/pymongo/mongo_client.py", line 1372, in _run_operation_with_response
exhaust=exhaust)
File "/root/miniconda-4.12.0/envs/emission/lib/python3.7/site-packages/pymongo/mongo_client.py", line 1471, in _retryable_read
return func(session, server, sock_info, slave_ok)
File "/root/miniconda-4.12.0/envs/emission/lib/python3.7/site-packages/pymongo/mongo_client.py", line 1366, in _cmd
unpack_res)
File "/root/miniconda-4.12.0/envs/emission/lib/python3.7/site-packages/pymongo/server.py", line 137, in run_operation_with_response
first, sock_info.max_wire_version)
File "/root/miniconda-4.12.0/envs/emission/lib/python3.7/site-packages/pymongo/helpers.py", line 168, in _check_command_response
max_wire_version)
pymongo.errors.OperationFailure: Internal server error, full error: {'ok': 0.0, 'operationTime': Timestamp(1681425547, 1), 'code': 42, 'errmsg': 'Internal server error'}

Note that we internally use AWS DocumentDB, a managed solution that is supposed to be compatible with MongoDB but sometimes has minor differences. We keep (and will continue to keep) the timeseries code consistent with both MongoDB and DocumentDB, but having people write arbitrary code in other modules makes it hard to find and fix all incompatibilities.

@AlirezaRa94
Copy link
Contributor

That's because the dynamic config was only done on 20th March (dbae8f9)

I've implemented dynamic config on this branch (#13), and this is just code refactoring.

@shankari
Copy link
Contributor

shankari commented Apr 14, 2023

I've implemented dynamic config on this branch (#13), and this is just code refactoring.

@AlirezaRa94 let's not get into a nitpicky argument about this. The dashboard code was not completely done by end of Feb, but neither was the NREL code for the survey. You are absolutely right that NREL had a significant delay in reviewing the code after it was written, at least in part because we were scrambling to finish the UI code changes.

Hopefully this is not the end of our collaboration, and we will continue to work together on improvements to both the survey and the dashboard.

Concretely, do you have an ETA for when the data access changes indb_util will be done?

We cannot deploy until they are complete. We can make the related changes ourselves - I wrote the entire server code, I can restructure two data access functions if I need to - but that will take our time away from the app UI changes. The whole point of the feature swap was that UNSW working on the dashboard would free up NREL resources to work on time use...

…uture

Before this

```
$ find . -name \*.py | xargs file
./globals.py:                      ASCII text
./utils/decode_jwt.py:             Python script text executable, ASCII text, with CRLF line terminators
./utils/constants.py:              ASCII text, with CRLF line terminators
./utils/generate_qr_codes.py:      Python script text executable, ASCII text
./utils/permissions.py:            Python script text executable, ASCII text, with CRLF line terminators
./utils/cognito_utils.py:          Python script text executable, ASCII text, with CRLF line terminators
./utils/db_utils.py:               Python script text executable, ASCII text, with CRLF line terminators
./utils/generate_random_tokens.py: Python script text executable, ASCII text
./globalsUpdater.py:               Python script text executable, ASCII text
./app.py:                          Python script text executable, ASCII text
./config-fake.py:                  ASCII text, with CRLF line terminators
./pages/home.py:                   Python script text executable, ASCII text
./pages/tokens.py:                 Python script text executable, ASCII text
./pages/map.py:                    Python script text executable, ASCII text
./pages/settings.py:               Python script text executable, ASCII text
./pages/push_notification.py:      Python script text executable, ASCII text
./pages/data.py:                   Python script text executable, ASCII text
./app_sidebar_collapsible.py:      Python script text executable, ASCII text, with CRLF line terminators
```

Auto-fix using

```
$ find . -name \*.py | xargs file | grep "with CRLF" | cut -d ":" -f 1 | xargs dos2unix
dos2unix: converting file ./utils/decode_jwt.py to Unix format...
dos2unix: converting file ./utils/constants.py to Unix format...
dos2unix: converting file ./utils/permissions.py to Unix format...
dos2unix: converting file ./utils/cognito_utils.py to Unix format...
dos2unix: converting file ./utils/db_utils.py to Unix format...
dos2unix: converting file ./config-fake.py to Unix format...
dos2unix: converting file ./app_sidebar_collapsible.py to Unix format...
```

After the change

```
$ find . -name \*.py | xargs file
./globals.py:                      ASCII text
./utils/decode_jwt.py:             Python script text executable, ASCII text
./utils/constants.py:              ASCII text
./utils/generate_qr_codes.py:      Python script text executable, ASCII text
./utils/permissions.py:            Python script text executable, ASCII text
./utils/cognito_utils.py:          Python script text executable, ASCII text
./utils/db_utils.py:               Python script text executable, ASCII text
./utils/generate_random_tokens.py: Python script text executable, ASCII text
./globalsUpdater.py:               Python script text executable, ASCII text
./app.py:                          Python script text executable, ASCII text
./config-fake.py:                  Python script text executable, ASCII text
./pages/home.py:                   Python script text executable, ASCII text
./pages/tokens.py:                 Python script text executable, ASCII text
./pages/map.py:                    Python script text executable, ASCII text
./pages/settings.py:               Python script text executable, ASCII text
./pages/push_notification.py:      Python script text executable, ASCII text
./pages/data.py:                   Python script text executable, ASCII text
./app_sidebar_collapsible.py:      Python script text executable, ASCII text
```
Convert all files to unix format to make code reviews easier in the f…
Follow up to 18c97c0
from #26

Also convert the docker-compose-prod.yml to unix endings
No other changes
So that we can test the reverse proxy configuration fixes in dev

This fixes #24 (comment)

The `docker-compose` (except for `nginx`) should be kept consistent with the
production `docker-compose-prod.yml` with the exception of `DASH_DEBUG_MODE`,
which is set to true so that we can debug errors in the setup more easily

Testing done:
- Built and ran the docker-compose
- Accessing http://localhost:8060/admin gives the same error as
#24 (comment)

```
Uncaught ReferenceError: DashRenderer is not defined
    <anonymous> http://localhost:8060/admin/:58
```
This internally sets the `requests_pathname_prefix`, which allows us to load
all the assets (including the javascript files) correctly. This is a partial
fix for #24
that implements the first step in
#24 (comment)
This fixes
#24 (comment)
#24 (comment)
#24 (comment)

Since there are not a lot of hrefs and they are all in the same file
#24 (comment)

```
$ grep -r href . | grep -v .git
./utils/cognito_utils.py:                dbc.Button('Login with AWS Cognito', id='login-button', href=CognitoConfig.AUTH_URL, style={
./app_sidebar_collapsible.py:                    href=dash.get_relative_path("/"),
./app_sidebar_collapsible.py:                    href=dash.get_relative_path("/data"),
./app_sidebar_collapsible.py:                    href=dash.get_relative_path("/tokens"),
./app_sidebar_collapsible.py:                    href=dash.get_relative_path("/map"),
./app_sidebar_collapsible.py:                    href=dash.get_relative_path("/push_notification"),
./app_sidebar_collapsible.py:                    href=dash.get_relative_path("/settings"),
```
Support and test the reverse proxy configuration
This is a hack to get the dashboard to work.
It retrieves data using the timeseries interfaces and performs the projections
using pandas locally.

It is *not* complete.

Notably:
- the projections are not the same as the original projections
- the projections do not take the dynamic config into account
- we still use `edb` for some calls - notably the ones to get the uuid table;
  we should add decorations to the timeseries to support them going forward
Hack to use the timeseries calls to retrieve the data
@shankari
Copy link
Contributor

@AlirezaRa94 @swastis10 This is a long and complicated PR already, so I am going to merge this as the first prototype release. It does meet all the minimum requirements:

  • login with AWS cognito
  • support trip table exports
  • support map display

I will then file a series of issues for cleanup as I see them, and for new functionality as we get feedback from program admins. Especially given that @AlirezaRa94 is now working on this project on a limited basis, I think it will be easier to track progress that way.

@shankari
Copy link
Contributor

shankari commented Apr 17, 2023

Current issues are:
Switch to the timeseries to read data instead of accessing mongodb directly: #29
Infer the config from the URL instead using the STUDY_NAME/STUDY_CONFIG environment variable: #31
Finalize production config: #32
Setup unit testing framework: #33

Those seem enough to be starting with; I will add more functionality-based issues as we test more.

@shankari shankari merged commit e94f3c4 into master Apr 17, 2023
@shankari
Copy link
Contributor

shankari commented Apr 17, 2023

I have also turned off write access for everybody except me and NREL IT (@asiripanich @AlirezaRa94 @swastis10), all further improvements will be through fork + pull request only. Please let me know if you have concerns about this.

Screenshot 2023-04-17 at 8 44 38 AM

Screenshot 2023-04-17 at 8 45 00 AM

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.

3 participants