search

qiita-spots / qiita

Qiita - A multi-omics databasing effort
http://qiita.microbio.me
BSD 3-Clause "New" or "Revised" License
119 stars 80 forks source link

Problem with qiita_env make #1216

Closed ElDeveloper closed 4 years ago

ElDeveloper commented 9 years ago

Calling qiita_env make --add-demo-user --load-ontologies --download-reference will fail with the following traceback:

  File "/Users/yoshikivazquezbaeza/git_sw/qiita/scripts/qiita_env", line 157, in <module>
    env()
  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-packages/click/core.py", line 610, in __call__
    return self.main(*args, **kwargs)
  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-packages/click/core.py", line 590, in main
    rv = self.invoke(ctx)
  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-packages/click/core.py", line 936, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-packages/click/core.py", line 782, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/yoshikivazquezbaeza/.virtualenvs/qiita/lib/python2.7/site-packages/click/core.py", line 416, in invoke
    return callback(*args, **kwargs)
  File "/Users/yoshikivazquezbaeza/git_sw/qiita/scripts/qiita_env", line 71, in make
    make_environment(load_ontologies, download_reference, add_demo_user)
  File "/Users/yoshikivazquezbaeza/git_sw/qiita/qiita_db/environment_manager.py", line 222, in make_environment
    _download_reference_files(conn)
  File "/Users/yoshikivazquezbaeza/git_sw/qiita/qiita_db/environment_manager.py", line 147, in _download_reference_files
    files['taxonomy'][0], files['tree'][0])
  File "/Users/yoshikivazquezbaeza/git_sw/qiita/qiita_db/reference.py", line 75, in create
    "filepath", conn_handler)[0]
  File "/Users/yoshikivazquezbaeza/git_sw/qiita/qiita_db/util.py", line 622, in insert_filepaths
    dd_id, mp = get_mountpoint(table)[0]
  File "/Users/yoshikivazquezbaeza/git_sw/qiita/qiita_db/util.py", line 564, in get_mountpoint
    basedir = get_db_files_base_dir()
  File "/Users/yoshikivazquezbaeza/git_sw/qiita/qiita_db/util.py", line 414, in get_db_files_base_dir
    "SELECT base_data_dir FROM settings")[0]
  File "/Users/yoshikivazquezbaeza/git_sw/qiita/qiita_db/sql_connection.py", line 519, in execute_fetchone
    with self._sql_executor(sql, sql_args) as pgcursor:
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()
  File "/Users/yoshikivazquezbaeza/git_sw/qiita/qiita_db/sql_connection.py", line 335, in _sql_executor
    (sql, str(sql_args), e)))
qiita_db.exceptions.QiitaDBExecutionError: 
Error running SQL query: SELECT base_data_dir FROM settings
ARGS: None
Error: relation "settings" does not exist
LINE 1: SELECT base_data_dir FROM settings

If you read the traceback, the problem is pointing to settings not existing in the database, however if you open a python session and try to use the function that's failing, you won't be able to reproduce the error:

In [1]: from qiita_db.util import get_db_files_base_dir

In [2]: get_db_files_base_dir()
'some path'

@josenavas suggested that this might be due to some uncommitted changes in the SQL connection, or maybe something else related to the fact that we only have one connection.

josenavas commented 9 years ago

I think the issue is this line: https://github.com/biocore/qiita/blob/master/qiita_db/environment_manager.py#L180

Since that connection is open with the database, we need to explicitly close that one, so any further connection to the DB re-opens it with a non-admin connection to a database.

ElDeveloper commented 9 years ago

Ah, that makes sense, let me try it out in my system.

On (May-28-15|12:29), josenavas wrote:

I think the issue is this line: https://github.com/biocore/qiita/blob/master/qiita_db/environment_manager.py#L180

Since that connection is open with the database, we need to explicitly close that one, so any further connection to the DB re-opens it with a non-admin connection to a database.

Reply to this email directly or view it on GitHub: https://github.com/biocore/qiita/issues/1216#issuecomment-106573624

ElDeveloper commented 9 years ago

I tried doing admin_conn.close() right before del admin_conn, but that didn't seem to work either, so I'm not sure what's going on.

On (May-28-15|12:29), josenavas wrote:

I think the issue is this line: https://github.com/biocore/qiita/blob/master/qiita_db/environment_manager.py#L180

Since that connection is open with the database, we need to explicitly close that one, so any further connection to the DB re-opens it with a non-admin connection to a database.

Reply to this email directly or view it on GitHub: https://github.com/biocore/qiita/issues/1216#issuecomment-106573624

ElDeveloper commented 9 years ago

Note that if I move the code, it the problem is not present anymore (not a real fix of course, but it's good to know):

diff --git a/qiita_db/environment_manager.py b/qiita_db/environment_manager.py
index 3fec1da..dc06373 100644
--- a/qiita_db/environment_manager.py
+++ b/qiita_db/environment_manager.py
@@ -209,6 +209,9 @@ def make_environment(load_ontologies, download_reference, add_demo_user):

     create_layout_and_patch(conn, verbose=True)

+    if download_reference:
+        _download_reference_files(conn)
+
     if load_ontologies:
         _add_ontology_data(conn)

@@ -218,9 +221,6 @@ def make_environment(load_ontologies, download_reference, add_demo_user):
         ontology = Ontology(convert_to_id('ENA', 'ontology'))
         ontology.add_user_defined_term('Amplicon Sequencing')

-    if download_reference:
-        _download_reference_files(conn)
-
     # we don't do this if it's a test environment because populate.sql
     # already adds this user...
     if add_demo_user and not qiita_config.test_environment:
josenavas commented 9 years ago

That is really interesting! Something in the ontology code?

wasade commented 9 years ago

Are you sure that this line issues a commit?

josenavas commented 9 years ago

All the execute calls in the SQLConnectionHandler commit, that's why we needed to create the queues.

wasade commented 9 years ago

Why the autocommit then?

wasade commented 9 years ago

Actually, suspect this line may be the issue. __del__ is not defined, so it is not explicitly closing the connection. Recommend using conn.close() as del will rely on the gc and this previously caused problems. We had to remove __del__ as the destructor was always closing the connection so we weren't getting the benefit of retaining a single open connection

josenavas commented 9 years ago

You're right. We had the __del__ defined before, but when we changed to a single connection, we remove that code. We should probably create a close function in the connection handler, so we can close the connection without accessing any to its attributes.

wasade commented 9 years ago

there is one...

wasade commented 9 years ago

Just to follow up, the method is here

ElDeveloper commented 8 years ago

This is still a problem and calling the close method doesn't work. At this point the solution is still to swap these statements as in the patch that I posted earlier in this thread. Would anyone oppose to me making that change and keep this issue open until we can figure out what the source of the error is?

antgonza commented 4 years ago

Closing as the DB connection code has been improved multiple times since this issue was opened and we haven't seen this error in a really long time; please open another issue if it happens again.