Skip to content
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

[Clean-up] Remove references to terminusdb #542

Merged
merged 13 commits into from
Jun 7, 2024
Merged

[Clean-up] Remove references to terminusdb #542

merged 13 commits into from
Jun 7, 2024

Conversation

PeopleMakeCulture
Copy link
Collaborator

Description

Removes references to terminusdb service not in use.

Fixes #513

Type of change

Clean-up

How Has This Been Tested?

None except CI Pipeline

@eecavanna
Copy link
Collaborator

This is the most aesthetically pleasing commit list I have ever seen! 📐

image

@PeopleMakeCulture
Copy link
Collaborator Author

PeopleMakeCulture commented May 30, 2024

Tests are failing. Do the tests expect terminusDB?
There is an issue with connecting the fastapi in the test env
wait-for-it.sh: timeout occurred after waiting 300 seconds for fastapi:8000

@eecavanna
Copy link
Collaborator

eecavanna commented May 30, 2024

Thanks, @PeopleMakeCulture. I am looking at the recent runs of the failing workflow.

image

It passed when it was run on the latest commit on main (which happened yesterday). But all the runs after that have failed.

image

As an experiment, I will re-run it on the tip of the main branch.

image

I'll report back with the result.


Update: It still passes on the tip of the main branch.

image

@eecavanna
Copy link
Collaborator

eecavanna commented May 30, 2024

Oops—my bad! The failures on the https://github.com/microbiomedata/nmdc-runtime/tree/523-queries-run-cmd-response-not-ok branch are pytest failures, not failures to connect to the FastAPI container. They just happen to occur as part of the same GHA workflow.

@eecavanna
Copy link
Collaborator

eecavanna commented May 30, 2024

I got a clue by checking this branch out locally and running $ make up-dev. When the fastapi container started up, its logs showed this error:

2024-05-30 16:27:06 INFO:     Will watch for changes in these directories: ['/code']
2024-05-30 16:27:06 INFO:     Uvicorn running on http://0.0.0.0:8000 (Press CTRL+C to quit)
2024-05-30 16:27:06 INFO:     Started reloader process [1] using WatchFiles
2024-05-30 16:27:14 Process SpawnProcess-1:
2024-05-30 16:27:14 Traceback (most recent call last):
2024-05-30 16:27:14   File "/usr/local/lib/python3.10/multiprocessing/process.py", line 314, in _bootstrap
2024-05-30 16:27:14     self.run()
2024-05-30 16:27:14   File "/usr/local/lib/python3.10/multiprocessing/process.py", line 108, in run
2024-05-30 16:27:14     self._target(*self._args, **self._kwargs)
2024-05-30 16:27:14   File "/usr/local/lib/python3.10/site-packages/uvicorn/_subprocess.py", line 78, in subprocess_started
2024-05-30 16:27:14     target(sockets=sockets)
2024-05-30 16:27:14   File "/usr/local/lib/python3.10/site-packages/uvicorn/server.py", line 62, in run
2024-05-30 16:27:14     return asyncio.run(self.serve(sockets=sockets))
2024-05-30 16:27:14   File "/usr/local/lib/python3.10/asyncio/runners.py", line 44, in run
2024-05-30 16:27:14     return loop.run_until_complete(main)
2024-05-30 16:27:14   File "uvloop/loop.pyx", line 1517, in uvloop.loop.Loop.run_until_complete
2024-05-30 16:27:14   File "/usr/local/lib/python3.10/site-packages/uvicorn/server.py", line 69, in serve
2024-05-30 16:27:14     config.load()
2024-05-30 16:27:14   File "/usr/local/lib/python3.10/site-packages/uvicorn/config.py", line 458, in load
2024-05-30 16:27:14     self.loaded_app = import_from_string(self.app)
2024-05-30 16:27:14   File "/usr/local/lib/python3.10/site-packages/uvicorn/importer.py", line 21, in import_from_string
2024-05-30 16:27:14     module = importlib.import_module(module_str)
2024-05-30 16:27:14   File "/usr/local/lib/python3.10/importlib/__init__.py", line 126, in import_module
2024-05-30 16:27:14     return _bootstrap._gcd_import(name[level:], package, level)
2024-05-30 16:27:14   File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
2024-05-30 16:27:14   File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
2024-05-30 16:27:14   File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
2024-05-30 16:27:14   File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
2024-05-30 16:27:14   File "<frozen importlib._bootstrap_external>", line 883, in exec_module
2024-05-30 16:27:14   File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
2024-05-30 16:27:14   File "/code/nmdc_runtime/api/main.py", line 29, in <module>
2024-05-30 16:27:14     from nmdc_runtime.api.endpoints import (
2024-05-30 16:27:14   File "/code/nmdc_runtime/api/endpoints/metadata.py", line 29, in <module>
2024-05-30 16:27:14     from nmdc_runtime.site.repository import repo, run_config_frozen__normal_env
2024-05-30 16:27:14   File "/code/nmdc_runtime/site/repository.py", line 27, in <module>
2024-05-30 16:27:14     from nmdc_runtime.site.graphs import (
2024-05-30 16:27:14   File "/code/nmdc_runtime/site/graphs.py", line 3, in <module>
2024-05-30 16:27:14     from nmdc_runtime.site.ops import (
2024-05-30 16:27:14 ImportError: cannot import name 'update_schema' from 'nmdc_runtime.site.ops' (/code/nmdc_runtime/site/ops.py)

For reference, here's what my environment has in it:

$ git log -1
commit ecb227d78a5465ab445df58ab067a4640bfadfc9 (HEAD -> 513-remove-tdb, origin/513-remove-tdb)
Author: J*** <j***@************>
Date:   Thu May 30 15:22:21 2024 -0400

    cleanup readme

$ git status
On branch 513-remove-tdb
Your branch is up to date with 'origin/513-remove-tdb'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   nmdc_runtime/site/translation/neon_benthic_translator.py
        modified:   nmdc_runtime/site/translation/neon_surface_water_translator.py

no changes added to commit (use "git add" and/or "git commit -a")

$ 

@eecavanna
Copy link
Collaborator

eecavanna commented May 30, 2024

@PeopleMakeCulture, here's a patch that fixes the ImportError I referred to in my previous comment.

$ git diff nmdc_runtime/site/graphs.py 
diff --git a/nmdc_runtime/site/graphs.py b/nmdc_runtime/site/graphs.py
index 09c5d51..5b21ac0 100644
--- a/nmdc_runtime/site/graphs.py
+++ b/nmdc_runtime/site/graphs.py
@@ -22,7 +22,6 @@ from nmdc_runtime.site.ops import (
     hello,
     mongo_stats,
     submit_metadata_to_db,
-    update_schema,
     filter_ops_undone_expired,
     construct_jobs,
     maybe_post_jobs,

I don't know whether that's the same underlying problem that is affecting the GHA workflow.

@PeopleMakeCulture
Copy link
Collaborator Author

PeopleMakeCulture commented Jun 6, 2024

@eecavanna thanks for pointing out the dependency on update_schema!

@PeopleMakeCulture
Copy link
Collaborator Author

PeopleMakeCulture commented Jun 6, 2024

@eecavanna That worked! Could you review/merge the PR and remove the terminus dependencies in rancher?

@eecavanna
Copy link
Collaborator

Great! I'll review the PR now.

@eecavanna
Copy link
Collaborator

Looks like a lot of files changed due to formatting changes only. I wonder whether this situation could be avoided in the future by running black on the entire codebase in a dedicated PR that is only about code formatting. Then, once that has been merged in, formatting-only changes wouldn't appear in the diffs of future PRs.

@eecavanna
Copy link
Collaborator

eecavanna commented Jun 7, 2024

I'm having a hard time reviewing this PR. @PeopleMakeCulture, will you make a new version of this PR that doesn't have the formatting-only changes in it? I don't know how time-consuming that would be at this point. I think it can be done by git cherry-pick-ing each commit other than 6f01714 (the one whose commit message is run black) into a new branch, and then opening a PR for that branch.

@eecavanna
Copy link
Collaborator

I just realized git cherry-pick preserves the commit's original author (I had assumed it didn't). In that case, I will try to create such a branch, myself (i.e. one that lacks the run black commit). I'll report back after I've tried.

eecavanna and others added 2 commits June 6, 2024 18:43
@eecavanna
Copy link
Collaborator

I created the branch I had in mind; and I opened a PR containing it. That PR is here: #549.

Then (oops), I realized I might be able to $ git revert that single run black commit from this PR's branch. I tried that and confirmed the result matches the branch I created in #549. So, I'll close PR #549.

Copy link
Collaborator

@eecavanna eecavanna left a comment

Choose a reason for hiding this comment

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

I am comfortable with this being merged in. Thanks, @PeopleMakeCulture, for implementing this!

P.S. To summarize my past few comments: I reverted the run black commit in an attempt to make the overall diff easier for me to read. The formatting of files unrelated to removing TerminusDB can be performed in a separate PR (that is what I prefer).

Copy link
Collaborator

@dwinston dwinston left a comment

Choose a reason for hiding this comment

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

great!

@dwinston dwinston merged commit f17fa64 into main Jun 7, 2024
@eecavanna eecavanna deleted the 513-remove-tdb branch June 7, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove obsolete code, env vars, container related to TerminusDB
3 participants