upgrade Python to v3.10 (iss. #1406) #1426

lsloan · 2022-09-07T03:37:07Z

To resolve vulnerability CVE-2015-20107, upgrade Python to v3.10. Also upgrade pandas related modules to work with the newer version of Python.

Minor changes were also made to start.sh to fix a spelling mistake and clean up trailing spaces on a few lines.

Closes #1406.

Test plan

Run MyLA locally and verify that it works properly.

Optional: Verify that Python 3.10 is installed.

docker exec -it student_dashboard python --version  # E.g., Python 3.10.6

Cleaned up trailing spaces on a few lines.

Resolves vulnerability CVE-2015-20107.

Upgrade `pandas` and related modules to work with Python 3.10.

ssciolla · 2022-09-07T16:31:41Z

requirements.txt

 numpy==1.22.0
-pandas==1.3.1
+pandas==1.4.4
 pangres==2.3.1


Seems like bumping numpy is also appropriate? pangres has had two major releases, so we may need to change API usages, but I'd look at that as well, since we're updating pandas.

Looks like this numpy version is still compatible. And pangres is pretty far behind, but seems like it should theoretically still be compatible. Did you test the cron?

Can you rebase this with master so we can more effectively test with the latest version of cron (UDP)? Thanks

When testing with UDW, I'm seeing this warning I wasn't seeing before when using pd.read_sql:

UserWarning: pandas only support SQLAlchemy connectable(engine/connection) ordatabase string URI or sqlite3 DBAPI2 connectionother DBAPI2 objects are not tested, please consider using SQLAlchemy

Looks like we're passing a Django connection object. I guess it's probably better to create our own SQLAlchemy engine and then close it at the end of the cron?

See these:

https://pandas.pydata.org/docs/reference/api/pandas.read_sql.html?highlight=read_sql#pandas.read_sql

https://stackoverflow.com/a/71083448

Actually, this is happening on the views, too. We'll need to think about how best to open/close connections. Maybe we can form a MySQL connection string? Alternatively, we can use the Django ORM to pull data in the views, and pass it into DataFrame constructors, but that's a bigger change.

I didn't test the cron.

Does the warning appear in the UI or in the server log?

If it doesn't appear in the UI, is the content affected by this warning? Is it getting the wrong information?

If the UI is unaffected, no warning or content problem, what should we do? Do we go ahead with this change on its own, or do we need to address the warning before including it in the release?

I'm okay with a warning filter.

Is this just a problem with the cron.py or were you seeing it in the application log from the views.py when using the django.db.connection? I guess I could run this and check.

I'm fine with either too, just seems like it's going to complicate this PR trying to implement something here for all the read_sql areas since we use 2 different databases.

I'm seeing it in the views.

Should be resolved by latest commits using the engine approach.

I also bumped numpy and pangres, pangres only required a small change to respond to a parameter name change

dashboard/cron.py

…ing-analytics into 1406-upgrade-python

Derive PostgreSQL connect string for data warehouse DB from Django connections, then create a single engine for all queries that will use that DB.

…y to views'

ssciolla · 2022-09-14T14:58:42Z

@jonespm, I implemented both of the quicker solutions discussed. Please let me know which you'd prefer and I'll either update this PR or create a new one:

jonespm · 2022-09-14T15:04:16Z

Was going to talk to you about this, thanks for picking this up @ssciolla

I'm good with #3, looks good. I'm fine with it being new after this is merged or just committed here and squashed.

Thanks! We'll probably want to improve the views in the future but we can probably wait to see if we have to rewrite them all anyway to use data marts.

jonespm · 2022-09-14T19:50:12Z

dashboard/common/db_util.py

+    PORT: int
+
+
+def create_sqlalchemy_engine(db_params: DBParams, type: Literal['mysql', 'postgres']) -> Engine:


It looks like the type parameter isn't really needed as ENGINE will always be available in DBParams if you added it to the DBParams class. There would be a value of.

django.db.backends.postgresql or django.db.bacakends.mysql.. So seems like you could just drop this parameter and use

if 'mysql' in new_db_params['ENGINE']:

But I guess it isn't a big deal either way.

I made the requested change, see d22125c

jonespm

I'd remove the type parameter but I'm fine with this either way. Looks good!

engine

…u#1426) * tl-its-umich-edu#1406 - spelling correction & clean-up Cleaned up trailing spaces on a few lines. * tl-its-umich-edu#1406 - upgrade to Python 3.10 Resolves vulnerability CVE-2015-20107. * tl-its-umich-edu#1406 - upgrade `pandas` for Python 3.10 Upgrade `pandas` and related modules to work with Python 3.10. * tl-its-umich-edu#1406 - fix all warehouse connections Derive PostgreSQL connect string for data warehouse DB from Django connections, then create a single engine for all queries that will use that DB. * Create utility function for creating mysql and postgres engines; apply to views' * Remove other database conn prep * Reuse create_sqlalchemy_engine in data_validation * Remove unused variable * Make a couple minor modifications to db_util * Make one read_sql call one line * Remove unused import * Update numpy, pangres; change mypy version * Remove type parameter, use Django ENGINE * Reverting change to validate_udw_vs_udp since it already created an engine Co-authored-by: Sam Sciolla <[email protected]> Co-authored-by: Code Hugger (Matthew Jones) <[email protected]>

lsloan added 3 commits September 6, 2022 23:28

tl-its-umich-edu#1406 - spelling correction & clean-up

4f2126a

Cleaned up trailing spaces on a few lines.

tl-its-umich-edu#1406 - upgrade to Python 3.10

36c418b

Resolves vulnerability CVE-2015-20107.

tl-its-umich-edu#1406 - upgrade pandas for Python 3.10

ce1c90f

Upgrade `pandas` and related modules to work with Python 3.10.

lsloan self-assigned this Sep 7, 2022

lsloan changed the title ~~⚠️ DRAFT - upgrade Python to v3.10 (iss. #1406)~~ upgrade Python to v3.10 (iss. #1406) Sep 7, 2022

lsloan marked this pull request as ready for review September 7, 2022 03:46

lsloan requested review from ssciolla and jonespm September 7, 2022 03:46

ssciolla reviewed Sep 7, 2022

View reviewed changes

lsloan commented Sep 13, 2022

View reviewed changes

dashboard/cron.py Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/tl-its-umich-edu/my-learn…

807ba64

…ing-analytics into 1406-upgrade-python

lsloan force-pushed the 1406-upgrade-python branch from 3dd6da0 to 807ba64 Compare September 13, 2022 19:46

lsloan and others added 4 commits September 13, 2022 15:53

tl-its-umich-edu#1406 - fix all warehouse connections

ce1f4d4

Derive PostgreSQL connect string for data warehouse DB from Django connections, then create a single engine for all queries that will use that DB.

Create utility function for creating mysql and postgres engines; appl…

995c64a

…y to views'

Remove other database conn prep

07632d9

Reuse create_sqlalchemy_engine in data_validation

efcb485

Remove unused variable

1f7cb30

ssciolla added 4 commits September 14, 2022 13:50

Make a couple minor modifications to db_util

91df38f

Make one read_sql call one line

2583a51

Remove unused import

5856f23

Update numpy, pangres; change mypy version

d5f3458

jonespm reviewed Sep 14, 2022

View reviewed changes

jonespm approved these changes Sep 14, 2022

View reviewed changes

ssciolla and others added 2 commits September 14, 2022 16:57

Remove type parameter, use Django ENGINE

d22125c

Reverting change to validate_udw_vs_udp since it already created an

8bc0bad

engine

ssciolla merged commit aa091fc into tl-its-umich-edu:master Sep 14, 2022

		PORT: int


		def create_sqlalchemy_engine(db_params: DBParams, type: Literal['mysql', 'postgres']) -> Engine:

upgrade Python to v3.10 (iss. #1406) #1426

upgrade Python to v3.10 (iss. #1406) #1426

Uh oh!

Conversation

lsloan commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssciolla Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ssciolla commented Sep 14, 2022

Uh oh!

jonespm commented Sep 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonespm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lsloan commented Sep 7, 2022 •

edited

Loading

ssciolla Sep 9, 2022 •

edited

Loading

jonespm left a comment •

edited

Loading