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

[SVCS-86] Add Papaya Renderer for Dicom and NifTi Files #265

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[#SVCS-86] Add papaya renderer for NifTI and DICOM files.
Add customization to papaya library to disallow opening of new files from user desktop.

Consulted with Product Team. They determined that it would be confusing
and possibly dangerous to allow opening of files from user desktop, when
they might confuse it with the WaterButler repository.

Used a custom built papaya.js built on

https://github.com/TomBaxter/Papaya/tree/feature/noNewFiles

I have sent them a PR, hopefully they bite.

rii-mango/Papaya#101
  • Loading branch information
TomBaxter committed Nov 17, 2017
commit fc577de07728b7dc02c15e91f518eb4ba4196ed2
1 change: 1 addition & 0 deletions mfr/extensions/papaya/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from .render import PapayaRenderer # noqa
48 changes: 48 additions & 0 deletions mfr/extensions/papaya/render.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import os
import time
import shutil

from mako.lookup import TemplateLookup

from mfr.core import extension
from mfr.extensions import settings as ext_settings
from mfr.extensions.papaya import settings


class PapayaRenderer(extension.BaseRenderer):

data_dir = settings.DATA_DIR
data_old = settings.DATA_OLD
Copy link
Contributor

Choose a reason for hiding this comment

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

How about data_exp and DATA_EXP for expiration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, OLD doesn't really fit. But EXP doesn't either. Went with TTL (Time To Live).

comp_ext = ext_settings.COMPRESSED_EXT

TEMPLATE = TemplateLookup(
directories=[
os.path.join(os.path.dirname(__file__), 'templates')
]).get_template('viewer.mako')

def render(self):
self.remove_old_files()
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean the temporary files for rendering will get stuck here all the time? Each time users try to render a file, the old ones gets removed but new ones are created.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is correct.

Copy link
Contributor

@cslzchen cslzchen Nov 6, 2017

Choose a reason for hiding this comment

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

Not sure how much this will impact prod memory. MFR and WB pods constantly run out of space though. I will leave this for Phase 2 Review.

file_name = os.path.basename(self.file_path)
if self.metadata.ext in self.comp_ext.keys():
second_ext = '.{}'.format(self.metadata.name.split('.')[-1])
if second_ext in self.comp_ext[self.metadata.ext]:
file_name = file_name + second_ext
file_name = file_name + self.metadata.ext
shutil.copyfile(self.file_path, self.data_dir + file_name)
return self.TEMPLATE.render(base=self.assets_url, file_name=file_name)

def remove_old_files(self):

for data_file in os.listdir(self.data_dir):
if data_file == '.gitignore':
continue
if (time.time() - os.path.getctime(self.data_dir + data_file)) >= self.data_old:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this a general util method for checking file time expiration for MFR in general?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

os.unlink(self.data_dir + data_file)

@property
def file_required(self):
return True

@property
def cache_result(self):
return True
8 changes: 8 additions & 0 deletions mfr/extensions/papaya/settings.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from mfr import settings

config = settings.child('PAPAYA_EXTENSION_CONFIG')

# Directory to temporarily store papaya image files
DATA_DIR = 'mfr/extensions/papaya/static/data/'
# Files older then this many seconds will be deleted from DATA_DIR at the beggining of each render.
DATA_OLD = 300
7 changes: 7 additions & 0 deletions mfr/extensions/papaya/static/README-papaya.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Currently using a custom built papaya.js with option to remove the ability to open new files from the users desktop.
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Remove this file. PR was accepted and we are no longer using custom build.

Copy link
Member

Choose a reason for hiding this comment

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

🎉 Nicely done!


https://github.com/TomBaxter/Papaya/tree/feature/noNewFiles

I have sent them a PR, hopefully they bite.

https://github.com/rii-mango/Papaya/pull/101
4 changes: 4 additions & 0 deletions mfr/extensions/papaya/static/data/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Ignore everything in this directory
*
# Except this file
!.gitignore
Copy link
Contributor

Choose a reason for hiding this comment

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

We should ignore the directory and all its sub directories and contents instead of having such a .gitignore. In this way, no need to check:

if data_file == '.gitignore':
    continue

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you're getting at here. In any case, the .gitignore was for my convenience while developing. Removing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, here is a directory /repo/level_1/level_2/level_3/. If you want to ignore all files in level_3/, instead of having a level3/.gitignore you can use a level2/.gitignore or even a repo level one that ignores everything in the directory level3/.

Copy link
Member Author

Choose a reason for hiding this comment

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

Travis failed, which reminded me of why I had the .gitignore. In addition to it being handy during development, I also need it to create and check in the empty directory mfr/extensions/papaya/static/data/

Copy link
Contributor

@cslzchen cslzchen Nov 7, 2017

Choose a reason for hiding this comment

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

👍 I will locally test it again today before sending this to PCR.

1 change: 1 addition & 0 deletions mfr/extensions/papaya/static/papaya.css

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading