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

Fix #1104: Improve lakeinfo structure and reporting #1319

Merged
merged 11 commits into from
Jul 3, 2024

Conversation

calina-c
Copy link
Contributor

@calina-c calina-c commented Jun 28, 2024

Closes #1104. Cherry picked from #1129.

TODO: In the meantime, we added tests for our dashboards. I think we should add Dash testing to this dashboard as well, but I would prefer doing it in a different PR.

@calina-c calina-c marked this pull request as ready for review July 3, 2024 09:48
@calina-c calina-c requested a review from trentmc July 3, 2024 09:48
@calina-c
Copy link
Contributor Author

calina-c commented Jul 3, 2024

Decided with Trent to add some basic unit tests to LakeInfo, in a separate PR.

@calina-c calina-c merged commit 4e06fa2 into main Jul 3, 2024
5 checks passed
@calina-c calina-c deleted the issue-1104-improve-lakeinfo-structure branch July 3, 2024 10:41
@idiom-bytes
Copy link
Member

idiom-bytes commented Jul 18, 2024

@trentmc @calina-c
I'm not sure why all of this was separated from the lake module?

[Problem - Core tooling is being moved to serve FE/webapps]
the core problem is that these tools exist to serve the lake module and help operate the lake along with drop, update, and many other functionalities...

the validations/info/debuggin/operational commands should remain inside of lake module... not moved into something that is to serve FE

[Recommendation - Separate core engine systems and code from webapp functionality]
I believe all frontend/webapp tools should exist in a separate module
/pdr_backend/ -> serves all the core engine systems, apis, etc..
/pdr_frontend/ -> provides all implementations for all webapp requirements

all core functionality for the web apps should then be referenced from pdr_backend

pytest should first run all coverage inside pdr_backend/ and then pdr_frontend/

[we now have 3 clis that are poorly named an structured]
cli/cli_module.py (main cli)
cli/cli_module_lake.py (lake cli)
lake_info/cli.py (lakeinfo cli)

the recommendations above would set a clear API/interface for how core engine functionality is separated from FE/webapp logic

@idiom-bytes
Copy link
Member

I have created a ticket requesting that we address this, including separating the webapp logic from engine logic, so we don't run into these problems in the future

#1419

@trentmc
Copy link
Member

trentmc commented Jul 18, 2024

The "pdr_backend/" directory is meant to house all the code, frontend and backend and everything in between. It's meant to mirror the name of the repo itself. The way that most well structured python repos work. And, needed for pypi. For historical reasons it has "backend" in the name. Let's not change that name right now, it would be a huge pain.

All cli stuff should be in pdr_backend/cli/. It can be cli to run lake, sim, sim plots, predictoor bots, trader bots, or dashboard.

And we have many support directories. Building blocks, if you will. Ppss, util, lake, aimodel, etc etc. They interlink though each has fairly well-defined responsibility.

We should 100% definitely NOT add a new directory parallel to pdr_backend. It would totally mess with the overall structure of the repo, including compromising how it needs to behave for pypi.

And there should definitely not be a directory named frontend/, anywhere. We have sim plots, we have the feed analysis app, and now we have the dashboard app. Each gets their own subdirectory of pdr_backend/, with an appropriate label.

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.

[Dash][App][Lake] Improve describe to be more user/debug friendly
3 participants