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

[WIP] Python3 support - Django #144

Closed
wants to merge 11 commits into from
Closed

Conversation

prshnt19
Copy link
Contributor

Fixes #121

@asmeurer
Copy link
Member

What is cloud_sql_proxy?

@prshnt19
Copy link
Contributor Author

Earlier AppEngine had its own database. But now we have to use Third-Party databases like MySQL hosted on Google Cloud. cloud_sql_proxy is used to connect to this Cloud MySQL shell from our device for development purposes.

@asmeurer
Copy link
Member

I ask because it's a 13 MB binary file. Is it necessary to include it in the repo?

@prshnt19
Copy link
Contributor Author

No, it's not necessary.

@asmeurer
Copy link
Member

You'll need to squash the commit that removes it, so that it isn't included in the git history.

@prshnt19
Copy link
Contributor Author

I have hosted it on AppEngine for testing. Works fine. Please have a look.
https://sympy-live-pr.appspot.com
ID : test
pass : test9876

@oscarbenjamin
Copy link
Contributor

I notice that underscore for the last output doesn't work e.g.:

>>> x = 2
>>> x
2
>>> 3*_
6

@asmeurer
Copy link
Member

asmeurer commented Mar 4, 2020

The completion code seems to be broken.

It may also be helpful if you push up a version of the docs to a github fork that points to this.

@prshnt19
Copy link
Contributor Author

prshnt19 commented Mar 5, 2020

I have made changes to the code.
'_' working.
completion working

@prshnt19
Copy link
Contributor Author

prshnt19 commented Mar 5, 2020

It may also be helpful if you push up a version of the docs to a github fork that points to this.

I didn't understand. Do I need to push docs for the changes I have made?

@asmeurer
Copy link
Member

asmeurer commented Mar 5, 2020

The easiest way will be to build a version of the SymPy docs that point to this (you will need to update the Sphinx extension and edit the doc/conf.py in sympy to point to the updated version), then build the docs, create a repo on GitHub with a gh-pages branch, and push the built docs there (copy _build/html there after building the docs). Is that clear? I can provide more detailed instructions if you need them.

@prshnt19
Copy link
Contributor Author

prshnt19 commented Mar 5, 2020

Thanks. Now it's clear. I will ask in case of any issue.

@prshnt19
Copy link
Contributor Author

prshnt19 commented Mar 7, 2020

It may also be helpful if you push up a version of the docs to a github fork that points to this.

prshnt19.github.io/sympy-live-docs

This is my copy of the sympy docs. This is pointing to:
https://sympy-live-pr.appspot.com for now.

Please review it once.

The admin panel of https://sympy-live-pr.appspot.com needs some static files to be integrated. For that, I have already opened a PR at sympy-web-static.

@prshnt19
Copy link
Contributor Author

prshnt19 commented Mar 9, 2020

ping @asmeurer

@oscarbenjamin
Copy link
Contributor

If you go here:
https://prashant.rawat.tk/sympy-live-docs/tutorial/basic_operations.html
and run the second block you'll see:

>>> expr = cos(x) + 1
>>> expr.subs(x, y)
Traceback (most recent call last):
  File "<string>", line 1, in <module>
AttributeError: module 'sympy.core.expr' has no attribute 'subs'

@asmeurer
Copy link
Member

I tried to verify the SymPy version with

import sympy
sympy.__version__

but I got the error NameError: name 'sympy' is not defined.

@prshnt19
Copy link
Contributor Author

https://sympy-live-pr.appspot.com does not support anonymous sessions for now. It needs a login. Anonymous sessions will resolve both issues. I am working on it.

@prshnt19
Copy link
Contributor Author

Anonymous User Issue fixed. You may test.

@sudoWin
Copy link

sudoWin commented Mar 14, 2020

This doesn't work

In [3]: A = Matrix([[12, -51, 4], [6, 167, -68], [-4, 24, -41]])                                                                                                                                            

In [4]: Q, R = A.QRdecomposition()

@prshnt19
Copy link
Contributor Author

prshnt19 commented Mar 14, 2020

Sympy has predefined Q. when we execute from sympy import *, Q is also imported. This error can be prevented by using q instead or reinitialising Q, like:

Q=1      # or anything else 
A = Matrix([[12, -51, 4], [6, 167, -68], [-4, 24, -41]])
Q, R = A.QRdecomposition()

@sudoWin
Copy link

sudoWin commented Mar 14, 2020

Ohh, So probably the example in the docs which has "Run Code Block in Sympy Live" also needs to be changed.

@prshnt19
Copy link
Contributor Author

Thanks for pointing this out.

@prshnt19
Copy link
Contributor Author

prshnt19 commented Mar 17, 2020

@asmeurer
Copy link
Member

Sorry I haven't had a chance to review this. I will try to do so soon.

Meanwhile you shouldn't let the lack of review here slow you down on any potential GSoC proposal.

Copy link
Member

@aktech aktech left a comment

Choose a reason for hiding this comment

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

@prshnt19 Thanks a lot for your work. To make the reviewing of this PR possible you need to update the README.md adding instructions for setting up the project. I see at least the following missing instructions:

  • Python version
  • Installing dependencies like: Mysql pre-requisites: https://pypi.org/project/mysqlclient/
    like say brew install mysql for Mac (pip install mysqlclient won't work otherwise)
  • Start mysql
  • Setup mysql user
  • Create sympyDB
  • migrate
  • Load static files

@prshnt19 prshnt19 force-pushed the python3_121 branch 2 times, most recently from 07fa62f to 0d5fa7b Compare April 12, 2020 20:54
@prshnt19 prshnt19 requested a review from aktech April 12, 2020 20:56
Copy link
Contributor Author

@prshnt19 prshnt19 left a comment

Choose a reason for hiding this comment

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

@aktech Thanks for your suggestions. I have updated the README. I have made some tweaks for static files so they are not needed to be loaded explicitly for testing.

Copy link
Member

@aktech aktech left a comment

Choose a reason for hiding this comment

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

I just had a look at it and here is my feedback:

I think this pull request is trying to solve multiple "problems" (also, may be the problem we don't have yet) at once like:

  • Changing the web framework
  • Upgrading Python2 -> Python3 (Hence changing the app engine runtime)
  • Adding a SQL database
    ...

First of all getting everything right and evaluating them at once is not a good idea and neither it is practical. Secondly, I am not convinced about using a sql database service like Cloud SQL for two reasons:

One it is a bit expensive and secondly we don't really need it as the things that we store in SQL is we can store in a columnar database like BigQuery which will cost almost nothing for our use case, as a consequence of this fact, Django's ORM is useless for us as it doesn't works with BigQuery, that would mean, we may not be able to use Django admin nor do I see any use case for the same.

So this brings me to the fundamental question, what problem is Django solving here? I read somewhere you mentioned session management is better in Django, I am not sure what you mean by that.

The suggestion is to creating a plan to upgrade one thing at a time, like for e.g. updating the things that are stopping us to migrate to Python 3 like say NDB Datastore. (To get an idea of what I mean, have a look at recent PRs in SymPy Gamma repo).

Also have a look at my comment in the mailing list here: https://groups.google.com/forum/#!topic/sympy/SQwTHhIZmxg

Comment on lines +13 to +23
- kind: Searches
properties:
- name: private
- name: timestamp
direction: desc

- kind: Searches
properties:
- name: user_id
- name: timestamp
direction: desc
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of these? I highly doubt that it is supported in Python3 runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was automatically generated while running dev_appserver for Python2 local testing. I think it should be removed.

README.rst Outdated
Grant all previleges to {user_name} on {database_name}::

mysql> CREATE DATABASE {database_name};
mysql> UPDATE mysql.user SET authentication_string = PASSWORD('{user_password}') WHERE User = '{user_name}';
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't works for me,

ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ...

I think this syntax varies with mysql version.

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 have updated the README. If it still doesn't work out for you please check MySQL documentation.

Comment on lines +6 to +7
numpy==1.18.1
scipy==1.4.1
Copy link
Member

Choose a reason for hiding this comment

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

Where do we use these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were to be used in case we make SymPy Live more generic. I think I should remove them and address that issue separately.

@prshnt19
Copy link
Contributor Author

This PR is a result of my attempt toward updating the Python2 to Python3 on Google AppEngine. AppEngine features changes with the selection of a language. For Python2 AppEngine uses its own libraries for DataBase, Users, etc. But while deploying with Python3 we have to use Third Party Libraries only. So to upgrade from Python2 to Python3 we also have to make a decision for Framework and DataBase of our own. This leaves us at implementing these task:

  • Upgrading Python2 -> Python3
  • Deciding Web framework
  • Adding a SQL database

So I went with Django and SQL. I used Cloud SQL for testing purpose because it was easy to set up, with no aim of using it in the long run. I am open to any alternatives. I haven't used BigQuery earlier but I do have some doubts about it. BigQuery is useful in cases where tables are large enough and row modification is not there. But our sessions will be storing data which will be modified with every statement we execute. So, in that case we might not be able to use BigQuery. Please correct me if I am wrong at some point.

I think updating the sessions with the pickled data is a bit easier in Django. Django gives many options to modify sessions with ease. With also being used in SymPy Gamma, I think Django is a better alternative to Flask. I also want to know the framework you think will be best suitable. The work on this PR is still in progress and a lot of things need to be figured out. Thanks again for your feedback.

@aktech
Copy link
Member

aktech commented Apr 17, 2020

This PR is a result of my attempt toward updating the Python2 to Python3 on Google AppEngine. AppEngine features changes with the selection of a language. For Python2 AppEngine uses its own libraries for DataBase, Users, etc. But while deploying with Python3 we have to use Third Party Libraries only. So to upgrade from Python2 to Python3 we also have to make a decision for Framework and DataBase of our own.

As I mentioned above, this PR is trying to solve multiple problems at once, the above doesn't needs to happen in one PR and it shouldn't as it will be hard to make right decisions and review, we need to do it one step at a time, by that I mean we need to prepare the app for it to migrate to Python 3.

So, for example currently this app uses bundled app engine services like datastore and uses Python2 NDB Client library to connect to it, so the first thing that needs upgrading is updating this to use Cloud NDB, which works on Python 2 as well, so we don't need to upgrade Python for this step. Similarly you can check out if there is any other bundled services it is using , which needs replacing and so on.

Here is a step by step guide:
https://cloud.google.com/appengine/docs/standard/python/migrate-to-python3/

So I went with Django and SQL. I used Cloud SQL for testing purpose because it was easy to set up, with no aim of using it in the long run.

Choosing framework would come up in later steps.

BigQuery is useful in cases where tables are large enough and row modification is not there. But our sessions will be storing data which will be modified with every statement we execute.

I don't mean to store things like sessions, which we constantly need to query in bigquery, its not great for reading in real time systems. For sessions we can still use Datastore, which we already use at the moment. If that's a problem or not it's a question for later when we have done the upgrade. Bigquery could be useful for storing search queries, the things we don't need to lookup, although I would still use Datastore for the same to start with and if it grows then moving to Bigquery later.

I think updating the sessions with the pickled data is a bit easier in Django. Django gives many options to modify sessions with ease. With also being used in SymPy Gamma, I think Django is a better alternative to Flask. I also want to know the framework you think will be best suitable. The work on this PR is still in progress and a lot of things need to be figured out. Thanks again for your feedback.

We will be more able to answer the choice of framework, when we have completed the steps preceding it, as it will be required when we move to Python3, which is a few steps away. My initial hunch is it won't matter much as Datastore is NoSQL database and Django doesn't supports NoSQL Database natively, but don't worry about it at the moment. (As a side note, we're discussing if we can afford Cloud SQL or not, as soon as we know, I will let you know.)

@aktech
Copy link
Member

aktech commented Apr 18, 2020

As a side note, we're discussing if we can afford Cloud SQL or not, as soon as we know, I will let you know.

To let you know, as per discussions with Aaron and Certik we have decided that we're not getting Cloud SQL as it doesn't justifies the cost at the moment.

@prshnt19
Copy link
Contributor Author

OK, I got your point for step by step updating. I will start working on updating to Cloud NDB first. While working on it We can figure out other bundles which need to be updated. Is there anything else which I might need to know before starting updating Cloud NDB? Or should I start working on it?

@aktech
Copy link
Member

aktech commented Apr 19, 2020

OK, I got your point for step by step updating. I will start working on updating to Cloud NDB first. While working on it We can figure out other bundles which need to be updated.

This link has all the information regarding incremental upgrade:
https://cloud.google.com/appengine/docs/standard/python/migrate-to-python3/

Is there anything else which I might need to know before starting updating Cloud NDB? Or should I start working on it?

Apart from what's mentioned in the above, link I cannot think of anything else. I worked on the Cloud NDB migration recently in Gamma, the following PR should help you get an idea of what needs to be done:

sympy/sympy_gamma#140

@aktech aktech closed this Jun 1, 2020
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.

Python 3 support
6 participants