learning-unlimited / ESP-Website

A website to help manage the logistics of large, short-term educational programs
82 stars 57 forks source link

Migration to Python3 #3616

Open milescalabresi opened 1 year ago

milescalabresi commented 1 year ago

Migrate the codebase to Python 3 (currently 3.7, aiming for higher once we bump Django and other packages)

hwatheod commented 1 year ago

Regarding Python 3.7, its end-of-life is only a few months way (2023-06-27).

milescalabresi commented 1 year ago

Good catch, Ted. I think we want to avoid upgrading Django in this PR (the main limiting library), but we should make a separate issue to bump Django as soon as possible. Then we can bump to a current version of Python. Only potential reason I can see to do it sooner is that this tool https://docs.python.org/3.10/library/functools.html#functools.cmp_to_key buys us a lot of ground with the cmp issue

milescalabresi commented 1 year ago

For the record, once the tests are fixed up and everything, you can test locally by running fab manage:test. EDIT: to test Python 3, see https://github.com/learning-unlimited/ESP-Website/pull/3411#issuecomment-919596499

hwatheod commented 1 year ago

Using the raw string is the right idea (so we don't need pickle at all), but you also need to get rid of the pickle when the value is read later (line 544 of the same file).

milescalabresi commented 1 year ago

Thanks @hwatheod. Do you think it's worth trying to use something like this https://opensource.com/article/20/11/django-rest-framework-serializers? Or just go with raw strings?

hwatheod commented 1 year ago

I think our objects are simple enough that it's overkill to use such serializers.

In this particular case, pickle is easy to remove because we pickle and then immediately unpickled on the other side. This is more-or-less the way pickle is intended to be used.

The real difficulty comes with the cases where pickled objects are stored in the database (which you are not supposed to do, but we did). That's where #3411 left off. It's worth reviewing all the discussion there on pickle again before tackling that case.

milescalabresi commented 1 year ago

I see. Maybe the django serializers could be used there? An alternative that would be a lot of work but is is cleaner at the end is to migrate the database and store the objects individually... I'll have to read those cases more closely

hwatheod commented 1 year ago

I would double check the documentation of the serializers to see if they are really intended for long term storage in a database, so we don't run into the same problems we did for pickle.

Pickle is intended as a short-term form of storage only, and even between one version of django and the next, pickles stored in a database can become unreadable because the pickle hard codes the names of classes into the serialized form, and django sometimes changes the names of classes from one version to the next. In the discussion in #3411 I mentioned I have found some pickled objects in our DBs from older versions of Django that can no longer be read by 1.11.

milescalabresi commented 11 months ago

To explain the previous commit a bit, Python 2 used the slash symbol / for integer division, unless one argument was already a float or Decimal. Python 3 assumes floating point division. Thus to "go back," modernize means changing single slashes to double slashes. However, it misses that "unless," so we have to look for obvious cases where one operand is a float. I used the decimal point and the float() constructor as signs. Terminal commands below, in case they help in the future.

find ./esp -name '*.py' | xargs git grep -n '\.[0-9].*[^\/\:]\/\/[^\/].*' # decimal point before division
find ./esp -name '*.py' | xargs git grep -n 'float.*[^\/\:]\/\/[^\/].*' # `float` constructor before division
find ./esp -name '*.py' | xargs git grep -n '[^\/\:]\/\/[^\/].*float' # `float` constructor after division
find ./esp -name '*.py' | xargs git grep -n '[^\/\:]\/\/[^\/].*\.[0-9]' # decimal point after division
hwatheod commented 4 months ago

I've been out of the loop on this PR for a while but it's great that you find a solution to the pickle migration problem. And also, cucumber is a great name for the util for working with pickles.

willgearty commented 3 months ago

When running fab setup to make my new VM, I had to do the following undocumented things:

  1. install setuptools
  2. hit enter twice to confirm various steps
willgearty commented 3 months ago

I also ran into the same error that @kkbrum encountered while trying to run fab setup:

ERROR: tablespace "encrypted" already exists

I tried dropping the tablespace, and got this warning:

WARNING: could not open directory "pg_tblspc/16384/PG_14_202107181": No such file or directory

Then I reran fab setup and got this error:

Device /dev/mapper/ubuntu--vg-keep_1 is in use. Cannot proceed with format operation.

milescalabresi commented 3 months ago

I made a new, clean VM for Ubuntu 24.04, so hoping that fixes things. Try again, reviewers!

willgearty commented 3 months ago

That seems to have worked @milescalabresi. I've got a python 3 dev server up and running now!

Although I will note that it seems to be running significantly slower than before. I realize that all of the caching needs to be done again, but even loading the same page or submitting a form takes a looooong time.

kkbrum commented 3 months ago

It worked for me as well. Mine seems a bit slower than before as well, but not as significantly as it seems to be for Will.