Fix metadata obj #1075

josenavas · 2015-04-17T10:26:04Z

This PR is built on top of #1074

This PR makes all the needed modifications in the metadata template objects to get all the tests under qiita_db/metadata_template/tests to pass. The PR right now is a bit big, but as the other PR in which this one depends gets merged, the size will be reduced.

antgonza · 2015-04-27T20:22:18Z

qiita_db/metadata_template/constants.py

+    'EBI': Restriction(columns={'collection_timestamp': 'timestamp',
+                                'physical_specimen_location': 'varchar'},
+                       error_msg="EBI submission disabled"),
+    'Qiita_main': Restriction(columns={'sample_type': 'varchar',


I'm not sure I like the casing. This is not an actual issue but looks weird and perhaps all should be lower case ...

lowercased!

What I don't like about this name is that "qiita main" isn't really meaningful.

Additionally the meaning of each of these keys should be documented somewhere.

What do you recommend for this? The restrictions under the qiita_main key are restrictions that only apply to our official Qiita main site, but they will not apply on a single user environment...

Re documentation: do you think it's enough to add a comment here?

Hmmm, then the restriction only applies to non-single-user systems right? As for better documenting this, I think we need something more than just a comment in the "constants" file. However I cannot think of a good way to do this.

Re documentation: I will add a comment here and I've created a new issue: #1111

I think I used the wrong words on my previous comment, the idea is that those restrictions only apply to our system. Other labs that may want to have a qiita installation locally with multiple users shouldn't have these restrictions either... Is one of those cases in which our system is more specialized than the rest of the users.

Thinking a bit more about this. I think that having this in a configuration file will allow us to change these restrictions dynamically, and we can have them documented there. What do you think?

Got it, yeah it makes more sense that way. Otherwise it is one of those
seemingly arbitrary restrictions of the system. If these are only for us
then they should be parametrized, although we do want to provide Qiita
as a service, the software should not be specific to our needs in any
way.

On (Apr-27-15|15:12), josenavas wrote:

@@ -6,10 +6,68 @@

The full license is in the file LICENSE, distributed with this software.

-----------------------------------------------------------------------------

+from collections import namedtuple
+from future.utils import viewkeys, viewvalues
+
+Restriction = namedtuple('Restriction', ['columns', 'error_msg'])
+
+# A dict containing the restrictions that apply to the sample templates
+SAMPLE_TEMPLATE_COLUMNS = {

'EBI': Restriction(columns={'collection_timestamp': 'timestamp',

'physical_specimen_location': 'varchar'},

error_msg="EBI submission disabled"),

'Qiita_main': Restriction(columns={'sample_type': 'varchar',

Re documentation: I will add a comment here and I've created a new issue: #1111

I think I used the wrong words on my previous comment, the idea is that those restrictions only apply to our system. Other labs that may want to have a qiita installation locally with multiple users shouldn't have these restrictions either... Is one of those cases in which our system is more specialized than the rest of the users.

Thinking a bit more about this. I think that having this in a configuration file will allow us to change these restrictions dynamically, and we can have them documented there. What do you think?

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

Yeah, I agree the idea of making this parametrized. Also, we don't know
when EBI decides to change their requirements. However, in interests of
time, I think we should create an issue and I can solve it as a part of
Alpha 0.2, since that will require some refactor. Does that sound good to
you?

2015-04-27 15:24 GMT-07:00 Yoshiki Vázquez Baeza notifications@github.com:

In qiita_db/metadata_template/constants.py
#1075 (comment):

@@ -6,10 +6,68 @@

The full license is in the file LICENSE, distributed with this software.

-----------------------------------------------------------------------------

+from collections import namedtuple
+from future.utils import viewkeys, viewvalues
+
+Restriction = namedtuple('Restriction', ['columns', 'error_msg'])
+
+# A dict containing the restrictions that apply to the sample templates
+SAMPLE_TEMPLATE_COLUMNS = {

'EBI': Restriction(columns={'collection_timestamp': 'timestamp',

'physical_specimen_location': 'varchar'},

error_msg="EBI submission disabled"),

'Qiita_main': Restriction(columns={'sample_type': 'varchar',

Got it, yeah it makes more sense that way. Otherwise it is one of those
seemingly arbitrary restrictions of the system. If these are only for us
then they should be parametrized, although we do want to provide Qiita as a
service, the software should not be specific to our needs in any way.
… <#14cfcfaf015149db_>
On (Apr-27-15|15:12), josenavas wrote: > @@ -6,10 +6,68 @@ > # The full

license is in the file LICENSE, distributed with this software. >

+from collections import namedtuple > +from future.utils import
viewkeys, viewvalues > + > +Restriction = namedtuple('Restriction',
['columns', 'error_msg']) > + > +# A dict containing the restrictions that
apply to the sample templates > +SAMPLE_TEMPLATE_COLUMNS = { > + 'EBI':
Restriction(columns={'collection_timestamp': 'timestamp', > +
'physical_specimen_location': 'varchar'}, > + error_msg="EBI submission
disabled"), > + 'Qiita_main': Restriction(columns={'sample_type':
'varchar', Re documentation: I will add a comment here and I've created a
new issue: #1111 I think I used
the wrong words on my previous comment, the idea is that those restrictions
only apply to our system. Other labs that may want to have a qiita
installation locally with multiple users shouldn't have these restrictions
either... Is one of those cases in which our system is more specialized
than the rest of the users. Thinking a bit more about this. I think that
having this in a configuration file will allow us to change these
restrictions dynamically, and we can have them documented there. What do
you think? --- Reply to this email directly or view it on GitHub:
https://github.com/biocore/qiita/pull/1075/files#r29197135

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

Jose Navas

Yeah, this sounds good we should resolve this before too long though.
Can you open an issue about this?

On (Apr-27-15|15:26), josenavas wrote:

@@ -6,10 +6,68 @@

The full license is in the file LICENSE, distributed with this software.

-----------------------------------------------------------------------------

+from collections import namedtuple
+from future.utils import viewkeys, viewvalues
+
+Restriction = namedtuple('Restriction', ['columns', 'error_msg'])
+
+# A dict containing the restrictions that apply to the sample templates
+SAMPLE_TEMPLATE_COLUMNS = {

'EBI': Restriction(columns={'collection_timestamp': 'timestamp',

'physical_specimen_location': 'varchar'},

error_msg="EBI submission disabled"),

'Qiita_main': Restriction(columns={'sample_type': 'varchar',

Yeah, I agree the idea of making this parametrized. Also, we don't know
when EBI decides to change their requirements. However, in interests of
time, I think we should create an issue and I can solve it as a part of
Alpha 0.2, since that will require some refactor. Does that sound good to
you?

2015-04-27 15:24 GMT-07:00 Yoshiki Vázquez Baeza notifications@github.com:

In qiita_db/metadata_template/constants.py
#1075 (comment):

@@ -6,10 +6,68 @@

The full license is in the file LICENSE, distributed with this software.

-----------------------------------------------------------------------------

+from collections import namedtuple
+from future.utils import viewkeys, viewvalues
+
+Restriction = namedtuple('Restriction', ['columns', 'error_msg'])
+
+# A dict containing the restrictions that apply to the sample templates
+SAMPLE_TEMPLATE_COLUMNS = {

'EBI': Restriction(columns={'collection_timestamp': 'timestamp',

'physical_specimen_location': 'varchar'},

error_msg="EBI submission disabled"),

'Qiita_main': Restriction(columns={'sample_type': 'varchar',

Got it, yeah it makes more sense that way. Otherwise it is one of those
seemingly arbitrary restrictions of the system. If these are only for us
then they should be parametrized, although we do want to provide Qiita as a
service, the software should not be specific to our needs in any way.
… <#14cfcfaf015149db_>
On (Apr-27-15|15:12), josenavas wrote: > @@ -6,10 +6,68 @@ > # The full

license is in the file LICENSE, distributed with this software. >

+from collections import namedtuple > +from future.utils import
viewkeys, viewvalues > + > +Restriction = namedtuple('Restriction',
['columns', 'error_msg']) > + > +# A dict containing the restrictions that
apply to the sample templates > +SAMPLE_TEMPLATE_COLUMNS = { > + 'EBI':
Restriction(columns={'collection_timestamp': 'timestamp', > +
'physical_specimen_location': 'varchar'}, > + error_msg="EBI submission
disabled"), > + 'Qiita_main': Restriction(columns={'sample_type':
'varchar', Re documentation: I will add a comment here and I've created a
new issue: #1111 I think I used
the wrong words on my previous comment, the idea is that those restrictions
only apply to our system. Other labs that may want to have a qiita
installation locally with multiple users shouldn't have these restrictions
either... Is one of those cases in which our system is more specialized
than the rest of the users. Thinking a bit more about this. I think that
having this in a configuration file will allow us to change these
restrictions dynamically, and we can have them documented there. What do
you think? --- Reply to this email directly or view it on GitHub:
https://github.com/biocore/qiita/pull/1075/files#r29197135

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

Jose Navas

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

Done #1113

2015-04-27 15:32 GMT-07:00 Yoshiki Vázquez Baeza notifications@github.com:

In qiita_db/metadata_template/constants.py
#1075 (comment):

@@ -6,10 +6,68 @@

The full license is in the file LICENSE, distributed with this software.

-----------------------------------------------------------------------------

+from collections import namedtuple
+from future.utils import viewkeys, viewvalues
+
+Restriction = namedtuple('Restriction', ['columns', 'error_msg'])
+
+# A dict containing the restrictions that apply to the sample templates
+SAMPLE_TEMPLATE_COLUMNS = {

'EBI': Restriction(columns={'collection_timestamp': 'timestamp',

'physical_specimen_location': 'varchar'},

error_msg="EBI submission disabled"),

'Qiita_main': Restriction(columns={'sample_type': 'varchar',

Yeah, this sounds good we should resolve this before too long though. Can
you open an issue about this?
… <#14cfd0256077b3f3_>
On (Apr-27-15|15:26), josenavas wrote: > @@ -6,10 +6,68 @@ > # The full

license is in the file LICENSE, distributed with this software. >

+from collections import namedtuple > +from future.utils import
viewkeys, viewvalues > + > +Restriction = namedtuple('Restriction',
['columns', 'error_msg']) > + > +# A dict containing the restrictions that
apply to the sample templates > +SAMPLE_TEMPLATE_COLUMNS = { > + 'EBI':
Restriction(columns={'collection_timestamp': 'timestamp', > +
'physical_specimen_location': 'varchar'}, > + error_msg="EBI submission
disabled"), > + 'Qiita_main': Restriction(columns={'sample_type':
'varchar', Yeah, I agree the idea of making this parametrized. Also, we
don't know when EBI decides to change their requirements. However, in
interests of time, I think we should create an issue and I can solve it as
a part of Alpha 0.2, since that will require some refactor. Does that sound
good to you? 2015-04-27 15:24 GMT-07:00 Yoshiki Vázquez Baeza <
notifications@github.com>: > In qiita_db/metadata_template/constants.py >
#1075 (comment): > > >
@@ -6,10 +6,68 @@ > > # The full license is in the file LICENSE,

distributed with this software. > >

+from collections import namedtuple > > +from future.utils import
viewkeys, viewvalues > > + > > +Restriction = namedtuple('Restriction',
['columns', 'error_msg']) > > + > > +# A dict containing the restrictions
that apply to the sample templates > > +SAMPLE_TEMPLATE_COLUMNS = { > > +
'EBI': Restriction(columns={'collection_timestamp': 'timestamp', > > +
'physical_specimen_location': 'varchar'}, > > + error_msg="EBI submission
disabled"), > > + 'Qiita_main': Restriction(columns={'sample_type':
'varchar', > > Got it, yeah it makes more sense that way. Otherwise it is
one of those > seemingly arbitrary restrictions of the system. If these are
only for us > then they should be parametrized, although we do want to
provide Qiita as a > service, the software should not be specific to our
needs in any way. > … <#14cfcfaf015149db_> > On (Apr-27-15|15:12),
josenavas wrote: > @@ -6,10 +6,68 @@ > # The full > license is in the file

LICENSE, distributed with this software. > # >

+from collections import namedtuple > +from future.utils import >
viewkeys, viewvalues > + > +Restriction = namedtuple('Restriction', >
['columns', 'error_msg']) > + > +# A dict containing the restrictions that
apply to the sample templates > +SAMPLE_TEMPLATE_COLUMNS = { > + 'EBI': >
Restriction(columns={'collection_timestamp': 'timestamp', > + >
'physical_specimen_location': 'varchar'}, > + error_msg="EBI submission >
disabled"), > + 'Qiita_main': Restriction(columns={'sample_type': >
'varchar', Re documentation: I will add a comment here and I've created a >
new issue: #1111 I think I used >
the wrong words on my previous comment, the idea is that those restrictions
only apply to our system. Other labs that may want to have a qiita >
installation locally with multiple users shouldn't have these restrictions
either... Is one of those cases in which our system is more specialized >
than the rest of the users. Thinking a bit more about this. I think that >
having this in a configuration file will allow us to change these >
restrictions dynamically, and we can have them documented there. What do >
you think? --- Reply to this email directly or view it on GitHub: >
https://github.com/biocore/qiita/pull/1075/files#r29197135 > > — > Reply
to this email directly or view it on GitHub > <
https://github.com/biocore/qiita/pull/1075/files#r29197966>. > -- Jose
Navas --- Reply to this email directly or view it on GitHub:
https://github.com/biocore/qiita/pull/1075/files#r29198175

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

Jose Navas

ElDeveloper · 2015-04-27T20:24:22Z

Can you pull from upstream?

…tadata-obj

…fix-metadata-obj

josenavas · 2015-04-27T20:36:40Z

Done!

ElDeveloper · 2015-04-27T20:37:12Z

@josenavas, from looking at the tests in Travis, it seems like there's a test case that's raising a warning that's not being catched, this might or not be intentional, but would be good to update the tests so there's no warnings being raised, see this for example:

/home/travis/build/biocore/qiita/qiita_db/metadata_template/base_metadata_template.py:583: QiitaDBWarning: Some functionality will be disabled due to missing columns:
    EBI submission disabled: primer.
Check https://github.com/biocore/qiita/wiki/Preparing-Qiita-template-files for a description of these fields.
  QiitaDBWarning)

josenavas · 2015-04-27T20:38:59Z

Ok, I'll update the tests so no warning is raised. There are quite a few instances of those... will ASAP.

ElDeveloper · 2015-04-27T20:52:09Z

qiita_db/metadata_template/base_metadata_template.py


        Returns
        -------
        md_template : DataFrame
            Cleaned copy of the input md_template
-
-        Raises


Can you add the warning being raised when there are missing columns?

Done, I was unsure if the warnings were documented in the same way :-)

antgonza · 2015-04-27T20:55:17Z

After these changes go in, do we need to update ftp://ftp.microbio.me/pub/qiita/sample_prep_template_examples.tgz ?

ElDeveloper · 2015-04-27T21:01:33Z

qiita_db/metadata_template/base_metadata_template.py

-        # Remove the sample_id and _id_column columns
-        db_cols.remove('sample_id')
-        db_cols.remove(cls._id_column)
+        headers = sorted(md_template.keys().tolist())


Do we no longer have to check the column names are unique? Or is this guaranteed by .keys?

At this point is it assumed that the md_template has been validated and clean (I think it's a safe assumption since this is a private function). Those checks are done in _clean_validate_template. That sounds reasonable?

Cool, thanks!

josenavas · 2015-04-27T21:23:19Z

@antgonza we probably will need to update those, good call. I will take a note to update those once the entire branch is ready to be merged in master.

I just found a small bug on the code and I'm fixing it. However, it requires modifying multiple tests. Will ping when I update everything (and including the comments made so far).

ElDeveloper · 2015-04-27T21:46:17Z

qiita_db/metadata_template/constants.py

+                              error_msg="Processed data approval disabled")
+}
+
+# A dict containing the restrictions that apply to the sample templates


sample -> prep

josenavas · 2015-04-27T22:19:50Z

@antgonza @ElDeveloper this should be ready for another review, thanks guys!

antgonza · 2015-04-27T22:22:10Z

qiita_db/metadata_template/base_metadata_template.py

+        if warning_msg:
+            warnings.warn(
+                "Some functionality will be disabled due to missing "
+                "columns:\n\t%s.\nCheck https://github.com/biocore/qiita/wiki"


Should this be a link so the user can access it?

How should I make it a link? Note that this error is raised in qiita_db. I'm hesitant to add specific html here since this warning can be seen from the CLI...

I was also thinking about this, but it's going to be tricky since this
can also be raised from a terminal and you would get an HTML formatted
error message, which is not appropriate. Ideas?

On (Apr-27-15|15:22), Antonio Gonzalez wrote:

# Retrieve the headers of the metadata template

headers = list(md_template.keys())

if warning_msg:

warnings.warn(

"Some functionality will be disabled due to missing "

"columns:\n\t%s.\nCheck https://github.com/biocore/qiita/wiki"

Should this be a link so the user can access it?

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

What's the issue with CLI? I think is fine ...

@ElDeveloper you're the CLI guru, what do you think? I prefer not to add HTML code in the CLI error/warning output, but if more people agrees I'm happy to add it...

I think this should be a separate conversation/issue. BTW IMOO the parsing should be in python/templates and not in JS.

That works!

On (Apr-27-15|16:29), Antonio Gonzalez wrote:

# Retrieve the headers of the metadata template

headers = list(md_template.keys())

if warning_msg:

warnings.warn(

"Some functionality will be disabled due to missing "

"columns:\n\t%s.\nCheck https://github.com/biocore/qiita/wiki"

I think this should be a separate conversation/issue. BTW IMOO the parsing should be in python/templates and not in JS.

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

So that means that this should stay as it is and we'll improve the qiita_pet error parsing?

yup, can you open an issue?

Sure! Done: #1115

2015-04-27 17:04 GMT-07:00 Antonio Gonzalez notifications@github.com:

In qiita_db/metadata_template/base_metadata_template.py
#1075 (comment):

# Retrieve the headers of the metadata template

headers = list(md_template.keys())

if warning_msg:

warnings.warn(

"Some functionality will be disabled due to missing "

"columns:\n\t%s.\nCheck https://github.com/biocore/qiita/wiki"

yup, can you open an issue?

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

Jose Navas

ElDeveloper · 2015-04-27T22:55:24Z

All my comments have been addressed, I only have a question, but that's not a blocking comment. @antgonza can you merge once your comments/questions are addressed?

josenavas · 2015-04-28T00:24:50Z

@antgonza can you merge if there are no outstanding issues?

josenavas · 2015-04-28T00:53:29Z

This is ready to go if there are no more comments!

ElDeveloper · 2015-04-28T01:10:29Z

LGTM.

Fix metadata obj

josenavas added 8 commits April 16, 2015 22:04

Adding the column_restriction module

8ec3b27

Fixing SampleTemplate ReadOnly tests

a13a7b8

Fixng all SampleTemplate tests

c64ab6c

Removing column restriction as it can be added in constants

5f23a55

Fixing TestPrepTemplateReadOnly

ef284f3

Fixing all prep template object tests

694f4ab

All tests under metadata_template passing

3728f63

Fixing flake8

b145fca

josenavas added refactor priority: high labels Apr 17, 2015

josenavas added this to the Alpha 0.1 milestone Apr 17, 2015

Fix merge conflicts

78f763a

josenavas mentioned this pull request Apr 24, 2015

Fix _almost_ all qiita db tests #1099

Merged

josenavas removed the priority: high label Apr 24, 2015

josenavas added 2 commits April 24, 2015 13:23

Merge branch 'fix-sample-obj' into fix-metadata-obj

013d126

Reading python patch as now it has the needed functionality

ba49dc5

antgonza reviewed Apr 27, 2015
View reviewed changes

josenavas added 3 commits April 27, 2015 13:34

Merge branch 'master' of https://github.com/biocore/qiita into fix-me…

57ed605

…tadata-obj

Merge branch 'relax-md-req' of https://github.com/biocore/qiita into …

2a29012

…fix-metadata-obj

Merge branch 'relax-md-req' of https://github.com/biocore/qiita into …

62f0d3e

…fix-metadata-obj

josenavas added the priority: high label Apr 27, 2015

ElDeveloper reviewed Apr 27, 2015
View reviewed changes

josenavas added 3 commits April 27, 2015 14:35

Fixing call to clean validate template

f81caa9

Removing all warnings from tests

134678d

Fixing create qiime mapping file add_filepath call

6aa5335

ElDeveloper reviewed Apr 27, 2015
View reviewed changes

Addressing comments

1cc26e9

antgonza reviewed Apr 27, 2015
View reviewed changes

Reducing the rename_cols dict

2d3df08

antgonza added a commit that referenced this pull request Apr 28, 2015

Merge pull request #1075 from josenavas/fix-metadata-obj

79256ff

Fix metadata obj

antgonza merged commit 79256ff into qiita-spots:relax-md-req Apr 28, 2015

Fix metadata obj #1075

Fix metadata obj #1075

Uh oh!

Conversation

josenavas commented Apr 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

The full license is in the file LICENSE, distributed with this software.

-----------------------------------------------------------------------------

Uh oh!

Choose a reason for hiding this comment

The full license is in the file LICENSE, distributed with this software.

-----------------------------------------------------------------------------

license is in the file LICENSE, distributed with this software. >

Uh oh!

Choose a reason for hiding this comment

The full license is in the file LICENSE, distributed with this software.

-----------------------------------------------------------------------------

The full license is in the file LICENSE, distributed with this software.

-----------------------------------------------------------------------------

license is in the file LICENSE, distributed with this software. >

Uh oh!

Choose a reason for hiding this comment

The full license is in the file LICENSE, distributed with this software.

-----------------------------------------------------------------------------

license is in the file LICENSE, distributed with this software. >

distributed with this software. > >

LICENSE, distributed with this software. > # >

Uh oh!

ElDeveloper commented Apr 27, 2015

Uh oh!

josenavas commented Apr 27, 2015

Uh oh!

ElDeveloper commented Apr 27, 2015

Uh oh!

josenavas commented Apr 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antgonza commented Apr 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas commented Apr 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas commented Apr 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment