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

Use python3 in marketplace #29

Merged
merged 124 commits into from
Sep 27, 2022
Merged

Use python3 in marketplace #29

merged 124 commits into from
Sep 27, 2022

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Aug 1, 2022

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).

@krzywon
Copy link
Contributor Author

krzywon commented Aug 24, 2022

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
Copy link
Contributor Author

krzywon commented Aug 24, 2022

@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
Copy link

@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
Copy link
Contributor Author

krzywon commented Aug 25, 2022

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
Copy link
Contributor Author

krzywon commented Aug 25, 2022

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
Copy link
Contributor Author

krzywon commented Aug 25, 2022

(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
Copy link
Contributor Author

krzywon commented Aug 25, 2022

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

@wpotrzebowski
Copy link

@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
Copy link
Contributor Author

krzywon commented Aug 29, 2022

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
Copy link
Contributor Author

krzywon commented Sep 7, 2022

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
Copy link

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
Copy link
Contributor Author

krzywon commented Sep 16, 2022

@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
Copy link
Member

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
Copy link
Contributor Author

krzywon commented Sep 19, 2022

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
Copy link
Member

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
Copy link

@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
Copy link
Contributor Author

krzywon commented Sep 20, 2022

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
Copy link
Contributor Author

krzywon commented Sep 20, 2022

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
Copy link

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

@butlerpd butlerpd merged commit 5f481a6 into master Sep 27, 2022
@butlerpd butlerpd deleted the python3 branch September 27, 2022 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants