Relax db #1073

josenavas · 2015-04-17T03:58:20Z

Note: this PR is made against the new relax-md-req.

I'm only changing the SQL layout and the populate_test_db.sql, so people can check if there is something terribly wrong.

I'm not changing any code, so tests are expected to fail.
In the next PR, (which will be build on top of this one) I'll start fixing the metadata_template objects, so some tests start passing with the new layout :-)

josenavas · 2015-04-21T18:02:26Z

@biocore/qiita-dev This is ready for review. Note that this is going to it's own branch. Tests are expected to fail. The only thing that I'm changing in this PR is the database layout and the populate test database file, so it can be successfully added after the new changes have been made.

josenavas · 2015-04-22T23:10:29Z

@biocore/qiita-dev The relax-md-req in upstream is now based on @squirrelo's changes, so this is modifying on top of his changes. Can be this be reviewed? Note that this is only changing the database, and the idea is get some feedback in terms of "don't do this is crazy". The earlier that I get that feedback the less burden I'll have later.

antgonza · 2015-04-23T19:15:28Z

qiita_db/support_files/patches/python_patches/23.py

+sql = """SELECT DISTINCT study_id from qiita.required_sample_info"""
+study_ids = set(s[0] for s in conn_handler.execute_fetchall(sql))
+
+queue_name = "PATCH_21"


queue name should be 23, right? Not blocking.

Good catch. Changed to avoid future confusions.

antgonza · 2015-04-23T19:27:25Z

Looks fine but it seems like all these changes can be done directly in a sql and not in python ...

josenavas · 2015-04-23T19:29:56Z

I can investigate on how to do everything on SQL. I'll dedicate a short period of time, otherwise I'm not going to meet the deadline. If you have a suggestion it will likely help me a lot to reduce development burden.

antgonza · 2015-04-23T19:33:44Z

I do not think is blocking, just curious why this choice?

josenavas · 2015-04-23T19:36:01Z

Shorter development time, I'm still getting used to more complex SQL queries, although I'm pretty exited on taking advantage of SQL.

antgonza · 2015-04-23T19:37:23Z

OK 👍 as we do not have any rules about this what should go in a sql or py patch.

josenavas · 2015-04-23T19:40:39Z

Great thanks

wasade · 2015-04-24T01:35:21Z

qiita_db/support_files/patches/python_patches/23.py

+# Since that table no longer stores required metadata,
+# we are going to rename it
+sql = """ALTER TABLE qiita.required_sample_info
+            RENAME TO study_sample"""


Is the schema implicit?

I will move this to SQL but just FYI, since you are changing the name of the table you don't need the schema. Actually, if you put the schema does not work (I've tested it on my system cause I also thought it was needed)

squirrelo · 2015-04-24T02:00:42Z

qiita_db/support_files/patches/python_patches/23.py

+# Since that table no longer stores common prep template metadata,
+# we are going to rename it
+sql = """ALTER TABLE qiita.common_prep_info
+            RENAME TO prep_template_sample"""


as @wasade said above, this probably needs schema in front of the name.

squirrelo · 2015-04-24T02:03:44Z

Few comments on the python patch. All in all, the logic on this is extremely hard to follow and I think it would be greatly improved if moved over to straight SQL. This would also take care of @wasade's concern about the selects being outside the queue.

wasade · 2015-04-24T02:08:17Z

@squirrelo, agree. I think it would be dangerous to have this patch run on a database that is not absolutely quiet (as no other connections except the one executing this patch).

josenavas · 2015-04-24T02:27:39Z

Thanks guys, I will try to do everything in SQL, those concerns are legitimate.

josenavas · 2015-04-24T19:43:51Z

@wasade @antgonza @squirrelo I moved everything to the SQL. Ready for review.

squirrelo · 2015-04-25T01:03:40Z

qiita_db/support_files/patches/23.sql

+                latitude=sv.latitude,
+                longitude=sv.longitude,
+                required_sample_info_status=sv.status
+            FROM sample_values sv


Why rename it here to sv when you can use WITH sv AS on line 52.

squirrelo · 2015-04-25T01:26:40Z

Python patch is broken:

  File "/Users/jshorens/Repositories/qiita/qiita_db/support_files/patches/python_patches/23.py", line 5, in <module>
    from qiita_db.metadata_template import SampleTemplate, PrepTemplate
  File "/Users/jshorens/Repositories/qiita/qiita_db/metadata_template/__init__.py", line 9, in <module>
    from .sample_template import SampleTemplate
  File "/Users/jshorens/Repositories/qiita/qiita_db/metadata_template/sample_template.py", line 23, in <module>
    from .prep_template import PrepTemplate
  File "/Users/jshorens/Repositories/qiita/qiita_db/metadata_template/prep_template.py", line 56, in <module>
    class PrepTemplate(MetadataTemplate):
  File "/Users/jshorens/Repositories/qiita/qiita_db/metadata_template/prep_template.py", line 70, in PrepTemplate
    id_cols_handlers = {'emp_status_id': get_emp_status()}
  File "/Users/jshorens/Repositories/qiita/qiita_db/util.py", line 268, in get_emp_status
    return dict(con.execute_fetchall(sql))
  File "/Users/jshorens/Repositories/qiita/qiita_db/sql_connection.py", line 454, in execute_fetchall
    with self._sql_executor(sql, sql_args) as pgcursor:
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()
  File "/Users/jshorens/Repositories/qiita/qiita_db/sql_connection.py", line 303, in _sql_executor
    (sql, str(sql_args), e)))
qiita_db.exceptions.QiitaDBExecutionError: 
Error running SQL query: select emp_status, emp_status_id from qiita.emp_status
ARGS: None
Error: relation "qiita.emp_status" does not exist
LINE 1: select emp_status, emp_status_id from qiita.emp_status

josenavas · 2015-04-25T01:28:50Z

Yes, python patch is broken because it relies on code that it needs to be added later. Just as a remainder, this is broken up a 44 file PR that we agreed was too hard to review. The system will not be fully functional until all PR go in the branch...

squirrelo · 2015-04-25T01:33:22Z

How are you testing the patch then? This needs to be complete for the patch to work at least, if it's going to be the actual patch PR.

josenavas · 2015-04-25T01:39:31Z

I've tested only the SQL patch, as the python patch is independent of the data in the database.

I've commented out the python patch for the time being, so I can add it back once the functionality that I need is in place. @squirrelo if you don't agree with that, I'll happy to do another 44 files PR and I hope you'll have fun reviewing it.

squirrelo · 2015-04-25T01:42:09Z

Since this is on a separate branch, can you please pull the python patch out of this PR and add it back in when it works, so we don't forget to test it later on.

josenavas · 2015-04-25T01:43:34Z

Done

squirrelo · 2015-04-27T16:20:02Z

👍

Relax db

josenavas added 3 commits April 16, 2015 20:01

Modifying the database

94bf806

Fixing populate_test_db.sql

b3aa78d

Renaming tables as the old names do not make sense now

fd13c59

josenavas added database changes refactor priority: high labels Apr 17, 2015

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

josenavas added 2 commits April 16, 2015 21:40

Correctly merging the column values in the sample template

7896e22

Fixing latitude/longitude types on populate

cc3b359

josenavas mentioned this pull request Apr 17, 2015

Fix sample obj #1074

Merged

Merge branch 'master' of https://github.com/biocore/qiita into relax-db

85a32dc

josenavas added 2 commits April 22, 2015 15:58

Preparing files for the merge

af248cf

Solving the hell of the merge conflict

b2721eb

antgonza reviewed Apr 23, 2015
View reviewed changes

Chaning queue name

299c52f

wasade reviewed Apr 24, 2015
View reviewed changes

squirrelo reviewed Apr 24, 2015
View reviewed changes

Doing all the patch in SQL

a8b7d98

squirrelo reviewed Apr 25, 2015
View reviewed changes

addressing @squirrelo's comments

5ee643f

Removing patch as per @squirrelo's suggestion

3d4cd42

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

Merge pull request #1073 from josenavas/relax-db

6177e4e

Relax db

antgonza merged commit 6177e4e into qiita-spots:relax-md-req Apr 27, 2015

Relax db #1073

Relax db #1073

Uh oh!

Conversation

josenavas commented Apr 17, 2015

Uh oh!

josenavas commented Apr 21, 2015

Uh oh!

josenavas commented Apr 22, 2015

Uh oh!

antgonza Apr 23, 2015

Choose a reason for hiding this comment

Uh oh!

josenavas Apr 23, 2015

Choose a reason for hiding this comment

Uh oh!

antgonza commented Apr 23, 2015

Uh oh!

josenavas commented Apr 23, 2015

Uh oh!

antgonza commented Apr 23, 2015 via email

Uh oh!

josenavas commented Apr 23, 2015

Uh oh!

antgonza commented Apr 23, 2015

Uh oh!

josenavas commented Apr 23, 2015

Uh oh!

wasade Apr 24, 2015

Choose a reason for hiding this comment

Uh oh!

josenavas Apr 24, 2015

Choose a reason for hiding this comment

Uh oh!

squirrelo Apr 24, 2015

Choose a reason for hiding this comment

Uh oh!

squirrelo commented Apr 24, 2015

Uh oh!

wasade commented Apr 24, 2015

Uh oh!

josenavas commented Apr 24, 2015

Uh oh!

josenavas commented Apr 24, 2015

Uh oh!

squirrelo Apr 25, 2015

Choose a reason for hiding this comment

Uh oh!

squirrelo commented Apr 25, 2015

Uh oh!

josenavas commented Apr 25, 2015

Uh oh!

squirrelo commented Apr 25, 2015

Uh oh!

josenavas commented Apr 25, 2015

Uh oh!

squirrelo commented Apr 25, 2015

Uh oh!

josenavas commented Apr 25, 2015

Uh oh!

squirrelo commented Apr 27, 2015

Uh oh!

Uh oh!