Massive file structure refactor; adding skeleton of FastAPI app #21

samuel-schwartz · 2025-01-22T22:13:10Z

Based on ideas here: https://docs.google.com/presentation/d/1jE0-VBikgAd-E6XSRTEkt_RxI190uVlsWg11fB6YgXw/edit?usp=sharing

d33bs

Nice job! I left a few comments for your consideration. Alongside the comments at the file level I also wondered more broadly:

Would it be possible to write a simple test (maybe using this fastapi tutorial as a reference point) which implements the server somehow to make sure it runs before it reaches "production"? Tests are commonly used to make sure an application can run bug-free before users reach it.
I noticed there were many environment files removed which could be important to other parts of the project (such as the front-end, or otherwise). As a double check: did these files present a challenge for the implementation you're suggesting with these changes? If not, I recommend re-including them to help make sure we don't lose capabilities which were already developed (and which could be important for this fastapi implementation).

d33bs · 2025-01-23T03:00:58Z

LICENSE

Consider retaining this file to help spell out the license associated with this project. Without this file the project effectively becomes completely private meaning others may not use it without explicit permission.

d33bs · 2025-01-23T03:01:43Z

CONTRIBUTING.md

Consider retaining this file to help spell out how contributions may be made to the project. Without it we risk misunderstandings when it comes to development procedures.

d33bs · 2025-01-23T03:04:06Z

hex/biz_logic/__init__.py

@@ -0,0 +1,11 @@
+from fastapi import APIRouter


Consider adding a docstring to the top of this file to help describe what it does. This comment applies to other Python modules within this PR which do not contain this pattern as well. It's common practice to provide a docstring at the top of a module to help describe what it does and to build context surrounding the other modules within a project.

d33bs · 2025-01-23T03:17:25Z

hex/administration/__init__.py

This comment applies to the directory in which this file is found. Consider organizing the source under this directory with a naming convention that follows package_name or src/package_name or app (where "package" in this sense can be understood to mean "a collection of Python modules"). These are common conventions used in many software projects to help indicate where the source code may be found. Since we also have a front-end to organize it might make sense to take the form: src/moss/backend and src/moss/frontend or something of this nature. As it stands without additional context one might understand the file organization to mean: "we're building a package called hex".

d33bs · 2025-01-25T20:57:42Z

README.md

-## Goal
-Here is an earlier iteration built using Kumu. We want to build something similar but better.
- - [kumu instance](https://embed.kumu.io/6cbeee6faebd8cc57590da7b83c4d457#default)
- - [demo video](https://www.youtube.com/watch?v=jZyLSRCba_M)


Consider somehow saving this information as it may still be useful in the immediate future as we migrate and change capability. I'm not 100% sure about this so it could be that this comment should be dismissed.

d33bs · 2025-01-25T21:06:50Z

security/database.py

@@ -0,0 +1,6 @@
+def get_db_connection(use_async=True):
+    TEMP_DB_LOC = "dev.db"


Consider giving this file an extension of .sqlite to explicitly observe that it is a SQLite database (and not some other database).

d33bs · 2025-01-25T21:10:35Z

security/database.py

@@ -0,0 +1,6 @@
+def get_db_connection(use_async=True):
+    TEMP_DB_LOC = "dev.db"
+    ASYNC_TEMP_DATABASE_URL = f"sqlite+aiosqlite:///./{TEMP_DB_LOC}"


I wasn't sure if the SQLite additions here were truly temporary or if they'd become the basis for later work. If it's more realistic that his might remain, consider assessing the performance constraints of SQLite in context with the size of the data involved for the project. I'd recommend DuckDB as one possible alternative with many similarities to SQLite and nice OLAP features which could compliment the project well.

d33bs · 2025-01-25T21:13:11Z

hex/biz_logic/harvest/endpoint.py

@@ -0,0 +1,103 @@
+from fastapi import APIRouter, Depends, HTTPException


Consider ensuring that fastapi is a part of the environment as a dependency which is declared somewhere (for example, in the pyproject.toml file).

d33bs · 2025-01-25T21:18:09Z

main.py

+This can be done by running the following command:
+
+python3 main.py


Consider adding this information to the readme or other documentation to help inform developers on how to run this. This could help someone who doesn't know which file to look at in order to start the app.

d33bs · 2025-01-25T21:26:45Z

mkdocs.yml

Consider keeping this file which looks to be part of mkdocs (likely used to help build documentation materials).

gpavlov2016 · 2025-01-28T16:24:43Z

hex/biz_logic/harvest/endpoint.py

+    return db_transaction
+    """
+    harvest = harvest.model_dump()
+    harvest["id"] = int(100*random())


Consider using UUID:
`import uuid

myuuid = uuid.uuid4()`

gpavlov2016 · 2025-01-28T16:44:41Z

I recommend adding some basic documentation for the API, example:

`/harvest`

Input Parameters:

repository name (str): Name of the repository to be registered.
submitter (str): Name or identifier of the person submitting the repository.

Output Parameters:

status (enum): Represents the status of the process. Possible values:
- in progress
- finished

Description:
Registers a code repository or a paper for ingestion (bottling).

Processing Steps:

Step 1: (Add step details here)
Step 2: (Add step details here)
... (Continue listing any additional steps)

samuel-schwartz added 4 commits January 8, 2025 13:02

File Refactoring

b367287

Filestructure refactor

4ae58ca

File structure refactor

fac751d

File structure refactor and minimal app model

fa81e92

samuel-schwartz requested review from d33bs, borisveytsman, pheochromo, gpavlov2016, jring-o and eyermt January 22, 2025 22:13

d33bs reviewed Jan 25, 2025

View reviewed changes

gpavlov2016 reviewed Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Massive file structure refactor; adding skeleton of FastAPI app #21

Massive file structure refactor; adding skeleton of FastAPI app #21

samuel-schwartz commented Jan 22, 2025

d33bs left a comment •

edited

Loading

d33bs Jan 23, 2025

d33bs Jan 23, 2025

d33bs Jan 23, 2025

d33bs Jan 23, 2025

d33bs Jan 25, 2025

d33bs Jan 25, 2025

d33bs Jan 25, 2025

d33bs Jan 25, 2025

d33bs Jan 25, 2025

d33bs Jan 25, 2025

gpavlov2016 Jan 28, 2025

gpavlov2016 commented Jan 28, 2025

		@@ -0,0 +1,6 @@
		def get_db_connection(use_async=True):
		TEMP_DB_LOC = "dev.db"

		@@ -0,0 +1,103 @@
		from fastapi import APIRouter, Depends, HTTPException

		This can be done by running the following command:

		python3 main.py

Massive file structure refactor; adding skeleton of FastAPI app #21

Are you sure you want to change the base?

Massive file structure refactor; adding skeleton of FastAPI app #21

Conversation

samuel-schwartz commented Jan 22, 2025

d33bs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpavlov2016 commented Jan 28, 2025

/harvest

d33bs left a comment •

edited

Loading

`/harvest`