Ebi step 3

[antgonza]

Sorry for the large amount of changes. Have no other idea of how to divide nicely, except making the DB changes in a different PR but that's small compared to the rest so decided to leave it here.

General comments:

• Added scientific name (required for EBI submission) to the populate_test_db.sql so we can test
• Remember that commands.submit_EBI is a work in progress and is just so we have an idea of how the calls will look
• Removed iter_file_via_list_of_dicts, add_sample, add_sample_prep and add_samples_from_templates as these were required for from-file submission, which we will not support anymore as everything is being loaded from the DB
• Removed _generate_library_descriptor as this was using/forcing to have some library design fields that are not required anymore
• Removed all write_*_xml as now we simply call the write/generate functions
• Removed write_all_xml_files cause this will defeat the idea of having networkx call/submit jobs. See point 2 of this list.
• Replaced gzopen -> GzipFile cause this will allow us to set mtime (float to calculate gzip's timestamp) for testing. FYI: gzip incorporates the timestamp in the file so we need to know/define the timestamp for testing.
• Removed all qiita_ware/test/test_data/* as now the files are generated and deleted on the fly.

Who wants to review?

[josenavas]

Instead of deleting this lines, can you just check that is returning an empty set? I think this was the original idea of this test but got lost when scientific_name become required. This way we will have a test for a template that is not missing anything.

[josenavas]

This comment is not accurate because you're actually checking the data type on line 153.

Anyways, I don't think the logic here is correct. In here we should be checking the columns required by EBI. In the current code, you're checking all the required columns, for multiple reasons - however I think that the correct thing to do is checking only the EBI columns.

Also, you don't need to actually check the data type - the templates have a property that will return the columns that are required for their datatype:

In [11]: st = SampleTemplate(1)

In [12]: st.columns_restrictions
Out[12]: 
{'EBI': Restriction(columns={'physical_specimen_location': 'varchar', 'collection_timestamp': 'timestamp', 'scientific_name': 'varchar', 'taxon_id': 'integer'}, error_msg='EBI submission disabled'),
 'qiita_main': Restriction(columns={'description': 'varchar', 'physical_specimen_remaining': 'bool', 'dna_extracted': 'bool', 'longitude': 'float8', 'sample_type': 'varchar', 'latitude': 'float8', 'host_subject_id': 'varchar'}, error_msg='Processed data approval disabled')}

In [13]: pt = PrepTemplate(1)

In [14]: pt.columns_restrictions
Out[14]: 
{'EBI': Restriction(columns={'center_name': 'varchar', 'platform': 'varchar', 'primer': 'varchar', 'experiment_design_description': 'varchar', 'library_construction_protocol': 'varchar'}, error_msg='EBI submission disabled'),
 'demultiplex': Restriction(columns={'primer': 'varchar', 'barcode': 'varchar'}, error_msg='Demultiplexing disabled. You will not be able to preprocess your raw data'),
 'demultiplex_multiple': Restriction(columns={'primer': 'varchar', 'barcode': 'varchar', 'run_prefix': 'varchar'}, error_msg='Demultiplexing with multiple input files disabled. If your raw data includes multiple raw input files, you will not be able to preprocess your raw data')}

Given this, I think the correct path here is:

restrictions = self.sample_template.columns_restrictions['EBI'].columns.keys()
restrictions.extend(self.prep_template.columns_restrictions['EBI'].columns.keys()

[antgonza]

I don't have a strong feeling about this just thought it was easier to remove than add. If nobody opposes I will only use EBI restrictions.

[josenavas]

Actually, thinking a bit more about this, you may be able to take advantage of the check_restrictions function from the templates:

pt_missing = self.prep_template.check_restrictions([self.prep_template.column_restrictions['EBI'])
st_missing = self.sample_template.check_restrictions([self.sample_template.column_restrictions['EBI'])

And then let the user know where are the columns missing (i.e. not only telling them which columns are missing but where):

if st_missing:
    # add error_msg for sample template
if pt_missing:
    # add error_msg for prep tempalte

[josenavas]

Thanks for the titanic effort here @antgonza ! I have done one pass and I think your errors will go away after addressing some of my comments

[ElDeveloper]

@antgonza, I think I'm done with my review. Looks good so far, but it would definitely be helpful if @adamrp reviewed this as well, there's a lot going on.

[mortonjt]

Since this isn't a private function would it be worthwhile to add the Parameters section in the numpydocs? (To clarify that s is a str)

[antgonza]

Yup.

[josenavas]

I know you didn't modify this, but can you reword this sentence so it fits in a single line in order to follow the numpy standard?

[mortonjt]

I did notice that in ebi.py is missing proper doc strings for send_sequences() and send_xml(). Do you guys mind if I fix this in a future PR?

[antgonza]

@mortonjt thanks for offering but those will be added in step 4 of this branch.

[antgonza]

Addressed all issues and questions. Can someone do a final review/merge?

[josenavas]

Sure, going over it again!

[josenavas]

It looks like my previous comment here was a bit confusing, sorry about that. The scientific_name should be 'root metagenome'.

'1118232' is the taxon_id (which is already present in the template).

IMO, not blocking - but would like to hear from @ElDeveloper

[ElDeveloper]

Not blocking at all.

[josenavas]

thanks @antgonza! few more comments.

I've added a couple of concerns to the joy list, as I think that this PR is getting to big and they can be fixed later and/or the final code will change once your work continues moving forward. In short, the issues that I'm concerned are:

• UTF-8 char errors (add test)
• Add a test in which the prep template has less samples than the sample template
• EBISubmission.__init__ is getting long and complex, so probably we want to revisit it. I can see this function changing once we move all the steps so I don't want to discuss it here.

[antgonza]

Thanks for adding. The other issues (not on the list) have been addressed.

[adamrp]

IMHO, the date_to_hold parameter should be renamed. To me, this name implies that the release should be held until today.

Also, this should default to None, then in the body of the function it should be set to date.today() (or today + 365 days) if it is None. Otherwise, this will not behave as you expect it to. As it is now, date.today() will be called only once, when the def is first executed (so, only once each time Qiita is launched, I assume), and the date_to_hold will be the same for all submissions, regardless of the actual submission date.

[adamrp]

A few comments on the code, but it looks good to me. It's great how much code can be deleted now that we are not relying at all on files and can instead use the data model! 😄

[adamrp]

these should be dashes (-) and not underscores (_) for numpydoc

[antgonza]

Ping! @adamrp @josenavas @mortonjt

[josenavas]

Really minor but EB.Submission.valid_platforms -> EBISubmission.valid_platforms

[josenavas]

Las commit was modifying only documentation - no reason to wait for the tests 😄 thanks @antgonza !!