unmtransinfo / CFChemApps

Web application for visualizing chemical structures
http://3.145.25.193/depict/
1 stars 0 forks source link

File read improvements #17

Closed Jack-42 closed 7 months ago

Jack-42 commented 8 months ago

This pull request addresses several things, including:

  1. Reporting errors from bad SMILES/SMARTS strings to an error log instead of throwing a server error
  2. Adding option to disable/enable molecular sanitation with a checkbox
  3. Adding user options for SMILES files (user can now specify delimiter, SMILES/Name column, if file has header),
  4. Added checks for input size (max 5 MB, but this can be easily changed)
  5. Allow for (.tsv, .smiles, and .txt) file types
  6. Setting a limit on how many molecules can be displayed at once (max 500, but this can be easily changed)
  7. Misc small improvements, including showing tooltips for different options and updated docs in the help page
  8. Removed unused files/functions, some code cleanup
  9. Cleaned up 'index.html' and moved JavaScript code to js files

Items 1 and 2 address Issue #12 . Overall, these changes give the user more flexibility and also help to provide more informative feedback rather than throwing "Server: 500" errors.

Priyansh-Kedia commented 8 months ago

The code follows a very clean pattern, looks good to me. Thanks for the changes @Jack-42

Can you please add a error.log file to store all the error messages that are encountered to be saved on the server.

Thanks

Jack-42 commented 8 months ago

I've added an error log file named depict.log. As well, in the backend we now save all files with unique names and delete a particular user's earlier image files on subsequent requests from that user.

I also modified the "Download" buttons so that the downloaded file will be renamed to the compound name (if it has a name) so that the user's experience isn't impacted by these changes.

Priyansh-Kedia commented 8 months ago

I've added an error log file named depict.log. As well, in the backend we now save all files with unique names and delete a particular user's earlier image files on subsequent requests from that user.

I also modified the "Download" buttons so that the downloaded file will be renamed to the compound name (if it has a name) so that the user's experience isn't impacted by these changes.

Hello @Jack-42 How are we tracking user requests? What is the way that we know which user is making request as we have no user information

Jack-42 commented 8 months ago

I've added an error log file named depict.log. As well, in the backend we now save all files with unique names and delete a particular user's earlier image files on subsequent requests from that user. I also modified the "Download" buttons so that the downloaded file will be renamed to the compound name (if it has a name) so that the user's experience isn't impacted by these changes.

Hello @Jack-42 How are we tracking user requests? What is the way that we know which user is making request as we have no user information

The variable imageFileNames and element created_filenames in index.html will hold the files uploaded. These will be unique to each user because javascript variables and html elements are local to each user's browser session.

I was thinking about it a bit and realized a drawback of this approach is that files created by the user won't be deleted when they exit the page. We could use a beforeunload event to do this, although I'd have to think about it some more.

Another option would be to routinely clear the media folder (say, by deleting all files that are older than 24 hrs) but right now I'm not sure of a good way to do that.

Priyansh-Kedia commented 8 months ago

I've added an error log file named depict.log. As well, in the backend we now save all files with unique names and delete a particular user's earlier image files on subsequent requests from that user. I also modified the "Download" buttons so that the downloaded file will be renamed to the compound name (if it has a name) so that the user's experience isn't impacted by these changes.

Hello @Jack-42 How are we tracking user requests? What is the way that we know which user is making request as we have no user information

The variable imageFileNames and element created_filenames in index.html will hold the files uploaded. These will be unique to each user because javascript variables and html elements are local to each user's browser session.

I was thinking about it a bit and realized a drawback of this approach is that files created by the user won't be deleted when they exit the page. We could use a beforeunload event to do this, although I'd have to think about it some more.

Another option would be to routinely clear the media folder (say, by deleting all files that are older than 24 hrs) but right now I'm not sure of a good way to do that.

Hi @Jack-42

We could store the generate image names in the request session in Django, inside the view function:

request.session['image_paths'] = <list of image paths>

And we could use middlware to clean up images after session expires in middlewares.py:

class CleanUpImagesMiddleware:
    def process_response(self, request, response):
        image_paths = request.session.pop('image_paths', [])
        // delete images here
        return response

and also adding the middlware in settings.py file

For our specific use case. I think this approach would work well. This way you would not need to store variables and elements in JS

Thanks

Jack-42 commented 8 months ago

Hey @Priyansh-Kedia

Thanks for your comments, I think this is indeed a good approach. I'm going to be out of town next week but will look into implementing this once I'm back from travel.

Jack-42 commented 7 months ago

Hey @Priyansh-Kedia

I added custom middleware that will remove all images generated by expired session(s). Upon a new request, CleanupImagesMiddleware will remove all images that were created by expired sessions and also delete the corresponding session since it's no longer needed. The JS elements/variables were removed.

In settings.py I set the expire time to 15 mins (i.e., a session expires after 15 mins of inactivity) but this value is flexible and can be changed. I tested the code in development mode and it seems to work as expected.

Before the next launch of production I can go in and manually remove older images in the media folder (since we weren't tracking them before). From then on, the media folder will be cleaned out automatically (every time someone opens the app/makes a request) using this new code.

Priyansh-Kedia commented 7 months ago

Hello @Jack-42 Thanks for implementing the middlware, the code looks good.

Please note: There is still an optimization consideration here, which instructs reducing the frequency of this cleanup to avoid impact on performance. However, we do not need that implementation currently.

Thanks

Jack-42 commented 7 months ago

Hello @Jack-42 Thanks for implementing the middlware, the code looks good.

Please note: There is still an optimization consideration here, which instructs reducing the frequency of this cleanup to avoid impact on performance. However, we do not need that implementation currently.

Thanks

Ok great, I'll go ahead and merge this and update production. Noted on the frequency of cleanup.