SasView / sasmodel-marketplace

A website for sharing custom sasmodel files
1 stars 2 forks source link

Use python3 in marketplace #29

Closed krzywon closed 1 year ago

krzywon commented 2 years ago

This should finalize the work started by @arm61 in PR #24 transitioning python2 -> python3. The development server is currently running off this branch.

Fixes #6 - The development server is running MySQL Fixes #22 - The development server is running python3 Fixes #23 - Uses the latest version of Django (v4.0.6 on the development server) Fixes #19 - 19 and 23 could almost be considered duplicate tickets No ticket - Use Github actions for deployment and unit testing (removing all Travis CI).

butlerpd commented 2 years ago

This needs review of both the code as well as the functionality. Besides moving from Python 2 to 3 it also is being moved to a server that can provide https.

krzywon commented 2 years ago

All data should now be ported and available on the development server. This includes user accounts, models, and downloadable model files.

wpotrzebowski commented 2 years ago

General question. I am looking at this statement at the main page: Contributed models should be written in Python (compatibility with at least python 2.7.x is required, but python 3.x is recommended) or, if computational speed is an issue, in a combination of Python and C. You only need to upload the .py/.c source code files to the Marketplace! I guess python 2 model files can still be viewed from the marketplace but not sure if they will work from sasview. I recall Paul K. added some functionality to support python 2 plugin models but not sure if it works anymore? As a general recomendation I guess we should suggest that all models should be written in python 3?

wpotrzebowski commented 2 years ago

I am not able to upload a model. Tested few py and c files from sasmodels models but each time it got an error Files of type text/x-python-script cannot be uploaded. Please select a file of type ['text/x-c', 'text/x-csrc', 'text/x-python', 'text/plain'].

krzywon commented 2 years ago

Upload doesn't seem to work

Thanks for testing, @wpotrzebowski. I'll take a look into file type allowances and run some tests locally.

krzywon commented 2 years ago

@wpotrzebowski, I just tested this myself and was able to upload a python file without issue. The mime type error you get with text/x-python-script seems to be the issue. The only known mimetype for python is text/x-python. How many different files have you tried?

My successful upload: https://marketplacedev.sasview.org/models/147/

wpotrzebowski commented 2 years ago

@wpotrzebowski, I just tested this myself and was able to upload a python file without issue. The mime type error you get with text/x-python-script seems to be the issue. The only known mimetype for python is text/x-python. How many different files have you tried?

My successful upload: https://marketplacedev.sasview.org/models/147/

I've tested at least 5 different model files and each time I was getting the same error. Could it be beacuse I am uploading from Mac (i.e. file has different type on Mac than on Windows)?

krzywon commented 2 years ago

I've tested at least 5 different model files and each time I was getting the same error. Could it be beacuse I am uploading from Mac (i.e. file has different type on Mac than on Windows)?

Looking deeper, the text/x-python-script is an allowed mime type for python files. I'll add it to the allowed list. Are you seeing this for c files too?

krzywon commented 2 years ago

Does build.yml workflow recreates database each time there is a push to repository? I am pretty sure it is not the case but I am thinking if any data is lost by this process?

The workflow does no deployment. It is used to ensure the build, migrations, and tests all work properly.

If desired, I could add a deployment step to push any changes to the server, but changes require a restart of apache to take effect, which would be safer to do from within the server.

krzywon commented 2 years ago

(compatibility with at least python 2.7.x is required, but python 3.x is recommended)

Suggested reword: Python v3.8 or higher is recommended. There is some backwards compatibility with python versions as early as 2.7.x, but older python code cannot be guaranteed to work in the latest version of SasView.

krzywon commented 2 years ago

@wpotrzebowski The changes are now applied to the development server. Can you try to upload a model when you get a chance?

wpotrzebowski commented 2 years ago

@krzywon Indeed, I can upload python files now! However, the identical/simillar error ocurs when trying to upload c file: Files of type application/octet-stream cannot be uploaded. Please select a file of type ['text/x-c', 'text/x-csrc', 'text/x-python', 'text/x-python-script', 'text/plain'].

krzywon commented 2 years ago

I'm leary of allowing any file of type application/octet-stream but also don't want to limit mac users to pure python models. This should be in the discussion for tomorrow.

krzywon commented 2 years ago

Here is the official definition for the mime type application/octet-stream - RFC 2046 4.5.1. I think my original concern is valid and we should not accept that type.

The "octet-stream" subtype is used to indicate that a body contains arbitrary binary data.

lucas-wilkins commented 1 year ago

Here is the official definition for the mime type application/octet-stream - RFC 2046 4.5.1. I think my original concern is valid and we should not accept that type.

I think it is probably correct to not accept it, however, the fix should probably be getting whatever is determining the mime type to correctly identify c source.

krzywon commented 1 year ago

@wpotrzebowski, I've changed how the mime checking is done. The mime is based on the file contents, using the python-magic package instead of the built-in content_type. Text-like files are more likely to be served as text/plain now, allowing a wider array of uploadable files. Please test file uploads on the dev server again.

My only question is at this point, should I limit the file extensions as well?

butlerpd commented 1 year ago

Not sure what you mean @krzywon? Are you suggesting we only allow .py and .c? I guess it may be good form to insist that a c file be .c (not .cpp :-) and that python be *.py? How do we upload the x,y data for the plot? That should allow any extension that is plain text?

krzywon commented 1 year ago

That's exactly what I'm asking, @butlerpd, but also allow .txt. As long as the python-magic package is doing its job, I don't think it's a good idea, but wanted a 2nd opinion.

butlerpd commented 1 year ago

I see. I guess I don't have a strong feeling either way. Maybe the answer then should be NOT to restrict until there is an actual identified problem in doing so?

wpotrzebowski commented 1 year ago

@krzywon I've tested .c files and they work fine (so as .py and .txt files). .txt files can also be added to models (not only as a part of example data). Is this is intended behavior - maybe is a part of your question? In my opinion we should only allow .c and .py extension in model section.

krzywon commented 1 year ago

Thanks for checking, @wpotrzebowski. Limiting the file extensions for the model files makes sense and I'll see if I can have that ready today.

krzywon commented 1 year ago

The dev server is now limiting model files to .py and .c extensions. No file extension limits are set for the example data because the data won't be stored if it is not multi-column ASCII text.

wpotrzebowski commented 1 year ago

Example data is limitted to text files and works as expected.