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

Treat each transcript id as an unique job identifier. #23

Merged
merged 25 commits into from
Sep 13, 2018

Conversation

cbaakman
Copy link
Contributor

@cbaakman cbaakman commented Aug 28, 2018

Makes sure that no two jobs can generate the same data file. The directory that holds the data is locked whenever modified and the celery id of the job is stored also.

The endpoints and javascript have been tested in the browser.

Copy link
Member

@laurensvdwiel laurensvdwiel left a comment

Choose a reason for hiding this comment

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

Please merge metadome/domain/services/metadome.py into this new job.py. Also check if the code from metadome.py is still needed?

@@ -0,0 +1,8 @@
import re
Copy link
Member

Choose a reason for hiding this comment

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

Please merge this into metadome/domains/services/helper_functions

@@ -60,11 +67,31 @@ def submit_gene_analysis_job_for_transcript(transcript_id):
return jsonify({'job_id': job_id,
'job_name': job_name,})


@bp.route('/submit_visualization/<transcript_id>/', methods=['GET'])
def submit_visualization_job_for_transcript(transcript_id):
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the other endpoints that tried to do this

@bp.route('/status', methods=['GET'])
def get_job_status_stub():
"""This endpoint is a stub, to ensure deeper endpoints may be used"""
pass


@bp.route('/status/<transcript_id>/', methods=['GET'])
def get_visualization_status_for_transcript(transcript_id):
Copy link
Member

Choose a reason for hiding this comment

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

This is 'get_job_status(job_name, job_id)', please remove the code if it is no longer needed

@@ -85,6 +112,22 @@ def get_job_result_stub():
"""This endpoint is a stub, to ensure deeper endpoints may be used"""
pass


@bp.route('/result/<transcript_id>/', methods=['GET'])
def get_visualization_result_for_transcript(transcript_id):
Copy link
Member

Choose a reason for hiding this comment

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

This is 'get_job_result(job_name, job_id):', please remove that method if this is the new way to do this

<span id="statusLabel"></span>
</div>

<div id="errorDisplay" class="container is-hidden">
Copy link
Member

Choose a reason for hiding this comment

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

I propose to remove this tag and use the templates/error.html

@@ -137,7 +137,15 @@
</div>
</div>
</div>


<div id="statusDisplay" class="container is-hidden">
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this tag, instead use this:

when loading:

$("#loading_overlay").addClass('is-active');

when finished:

$("#loading_overlay").removeClass('is-active');

if (this.readyState == 4 && this.status > 0) {
var response = JSON.parse(xhttp.responseText);
if (response.error !== undefined)
showErrorOnPage(response.error);
Copy link
Member

Choose a reason for hiding this comment

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

To error page

Copy link
Member

Choose a reason for hiding this comment

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

error.html redirect

xhttp.send();
}

function hideStatusOnPage() {
Copy link
Member

Choose a reason for hiding this comment

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

no longer needed

$("#statusDisplay").addClass('is-hidden');
}

function showStatusOnPage(status) {
Copy link
Member

Choose a reason for hiding this comment

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

No longer needed

$("#statusLabel").text("status for transcript " + pageTranscriptID + ": " + status);
}

function hideErrorOnPage() {
Copy link
Member

Choose a reason for hiding this comment

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

No longer needed

metadome/presentation/web/templates/js/dashboard.js Outdated Show resolved Hide resolved
Copy link
Member

@laurensvdwiel laurensvdwiel left a comment

Choose a reason for hiding this comment

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

I added two comments, please check if you are able to fix them :)

_log.info("visualization job for transcript {} is already running as task {}"
.format(transcript_id, task_id))
else:
from metadome.tasks import create_prebuild_visualization
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to create the file here that you are about to lock, now multiple tasks for the same gene-transcript could be submitted by opening a second tab in the browser and performing the same query. Try the gene ZNF479 (will take a long time to run) for example.

function getVisualizationStatus(transcript_id) {
$.get(Flask.url_for("api.get_visualization_status_for_transcript",
{'transcript_id': transcript_id}),
function(data) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a way to manipulate the text of this label: https://github.com/cmbi/metadome/blob/master/metadome/presentation/web/templates/dashboard.html#L32

And after 5 status checks of 10 ms, change the text to: "Loading ...
This is taking longer than usual, and may take up to an hour. You can choose to wait or check back later." and change the checks to 50ms

Also change the text back to 'Loading ...' after success, as the overlay is used at other points in the visualization as well.

Copy link
Member

@laurensvdwiel laurensvdwiel left a comment

Choose a reason for hiding this comment

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

Tested changes and approved. Thanks @cbaakman this looks really good :) 👍

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.

2 participants