Skip to content

Delete preprocess data #1042

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

Merged
merged 6 commits into from
Apr 8, 2015
Merged
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
Prev Previous commit
Next Next commit
deleting code to qiita_db
  • Loading branch information
antgonza committed Apr 7, 2015
commit 656128f3c1408c71e01d00a32d7637e57d0ca81a
68 changes: 67 additions & 1 deletion qiita_db/data.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class BaseData(QiitaObject):
--------
RawData
PreprocessedData
PreprocessedData
ProcessedData
"""
_filepath_table = "filepath"

Expand Down Expand Up @@ -807,6 +807,72 @@ def create(cls, study, preprocessed_params_table, preprocessed_params_id,
ppd.add_filepaths(filepaths, conn_handler)
return ppd

@classmethod
def delete(cls, ppd_id):
"""Removes the preprocessed data with id ppd_id

Parameters
----------
ppd_id : int
The preprocessed data id

Raises
------
QiitaDBStatusError
If the preprocessed data status is not sandbox or if the
preprocessed data EBI and VAMPS submission is not in a valid state
['not submitted', 'failed']
QiitaDBError
If the preprocessed data has (meta)analyses
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not accurate: If the preprocessed data has been processed

"""
valid_submission_states = ['not submitted', 'failed']
ppd = cls(ppd_id)
if ppd.status != 'sandbox':
raise QiitaDBStatusError(
"Illegal operation on non sandbox preprocessed data")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these messages (832 and 836) are likely to be seen by users on the GUI, do you think you could rephrase them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the changes. I think it should be "sandboxed" not "sandbox".

elif (ppd.submitted_to_vamps_status() not in valid_submission_states or
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you break this into two if statements? That way we know which is actually the error, VAMPS or INSDC status.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

On (Apr-07-15|10:58), Joshua Shorenstein wrote:

  •    Raises
    

  •    QiitaDBStatusError
    
  •        If the preprocessed data status is not sandbox or if the
    
  •        preprocessed data EBI and VAMPS submission is not in a valid state
    
  •        ['not submitted', 'failed']
    
  •    QiitaDBError
    
  •        If the preprocessed data has been processed
    
  •    """
    
  •    valid_submission_states = ['not submitted', 'failed']
    
  •    ppd = cls(ppd_id)
    
  •    if ppd.status != 'sandbox':
    
  •        raise QiitaDBStatusError(
    
  •            "Illegal operation on non sandbox preprocessed data")
    
  •    elif (ppd.submitted_to_vamps_status() not in valid_submission_states or
    

Can you break this into two if statements? That way we know which is actually the error, VAMPS or INSDC status.


Reply to this email directly or view it on GitHub:
https://github.com/biocore/qiita/pull/1042/files#r27903143

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

ppd.submitted_to_insdc_status() not in valid_submission_states):
raise QiitaDBStatusError(
"Illegal operation due to EBI submission status ('%s') or "
"VAMPS submission status ('%s')" % (
ppd.submitted_to_insdc_status(),
ppd.submitted_to_vamps_status()))

conn_handler = SQLConnectionHandler()

processed_data = [str(n[0]) for n in conn_handler.execute_fetchall(
"SELECT processed_data_id FROM qiita.preprocessed_processed_data "
"WHERE preprocessed_data_id = {0} ORDER BY "
"processed_data_id".format(ppd_id))]

if processed_data:
raise QiitaDBError(
"Preprocessed data %d cannot be removed because it was used "
"to generate the following processed data: %s" % (
ppd_id, ', '.join(processed_data)))

# delete
queue = "delete_preprocessed_data_%d" % ppd_id
conn_handler.create_queue(queue)

sql = ("DELETE FROM qiita.prep_template_preprocessed_data WHERE "
"preprocessed_data_id = {0}".format(ppd_id))
conn_handler.add_to_queue(queue, sql)

sql = ("DELETE FROM qiita.preprocessed_filepath WHERE "
"preprocessed_data_id = {0}".format(ppd_id))
conn_handler.add_to_queue(queue, sql)

sql = ("DELETE FROM qiita.study_preprocessed_data WHERE "
"preprocessed_data_id = {0}".format(ppd_id))
conn_handler.add_to_queue(queue, sql)

sql = ("DELETE FROM qiita.preprocessed_data WHERE "
"preprocessed_data_id = {0}".format(ppd_id))
conn_handler.add_to_queue(queue, sql)

conn_handler.execute_queue(queue)

@property
def processed_data(self):
r"""The processed data list generated from this preprocessed data"""
Expand Down
32 changes: 32 additions & 0 deletions qiita_db/test/test_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,38 @@ def test_create_data_type_only(self):
# preprocessed_data_id, filepath_id
self.assertEqual(obs, [[3, obs_id - 1], [3, obs_id]])

def test_delete(self):
"""Correctly deletes a preprocessed data"""
# testing regular delete
ppd = PreprocessedData.create(
self.study, self.params_table,
self.params_id, self.filepaths, prep_template=self.prep_template,
ebi_submission_accession=self.ebi_submission_accession,
ebi_study_accession=self.ebi_study_accession)
PreprocessedData.delete(ppd.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have some SQL tests to make sure the delete actually removed all the rows expected from the DB.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add but I see them as useless as the test just after this one is to check that the deletion fails cause this object doesn't exist anymore, do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to make sure that the lines are all deleted out of the 4 or 5 tables, since the failure is only due to one row in one table.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the lines are in a queue so if one fails, all fail, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's for future-proofing to make sure the lines are always deleted no matter the code changes. If we change the code, we still need to make sure the functionality remains.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point @antgonza. However, this won't work in the case that we move the "delete" function from a classmethod to an instance method. This is something that I've been thinking and I was planning to propose to the group, as I think it makes way more sense to have the delete function as an instance method rather than a classmethod.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you feel like that's what we should do, IMOO and following the "rules" we discussed last week, that should be a separate issue. Anyway, the concern about having it as instance method is:

pd.delete()
pd.whatever = 55

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree that it should be a separate issue. BTW, that concern is not solved with the current solution:

PreprocessedData.delete(ppd.id)
ppd.whatever = 55

I moved the discussion here: #1045

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, I think that @ElDeveloper enhancements should be implemented, as it is safer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @ElDeveloper enhancements as well.


# testing that it raises an error if ID doesn't exist
with self.assertRaises(QiitaDBUnknownIDError):
PreprocessedData.delete(ppd.id)

# testing that we can not remove cause the preprocessed data != sandbox
with self.assertRaises(QiitaDBStatusError):
PreprocessedData.delete(1)

# testing that we can not remove cause preprocessed data has been
# submitted to EBI or VAMPS
pd = ProcessedData(1)
pd.status = 'sandbox'
with self.assertRaises(QiitaDBError):
PreprocessedData.delete(pd.preprocessed_data)

# testing that we can not remove cause preprocessed data has processed
# data
ppd = PreprocessedData(pd.preprocessed_data)
ppd.update_insdc_status('not submitted')
with self.assertRaises(QiitaDBError):
PreprocessedData.delete(ppd.id)

def test_create_error_dynamic_table(self):
"""Raises an error if the preprocessed_params_table does not exist"""
with self.assertRaises(IncompetentQiitaDeveloperError):
Expand Down