Skip to content

Orchestration - #24

Open
joshuaRiefman wants to merge 20 commits into
mainfrom
orchestration
Open

joshuaRiefman wants to merge 20 commits into
mainfrom
orchestration

Conversation

@joshuaRiefman

Copy link
Copy Markdown
Member

[FEAT/FIX/DOCS]: Pull Request Name

  • Module Documentation
  • Method/Class/Function Documentation
  • Pull Request Completed

What's New

[What has been added?]

Bugfixes

[Have you added any bug fixes?]

Deprecated/Removed

[Has anything been removed or deprecated?]

Dependencies

[Any changes in dependencies?]

Notes

[Anything else relevant?]

@jonahjlee jonahjlee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR is too large for me to review in detail, but I read the docs and skimmed through the code and it looks good.

Committing API tokens feels strange even though it's safe in our case...

In terms of the orchestration module: it seems to be working well, but I think that it's a more complex solution that what we really need, since we will probably not be running offline Sunbeam very often. Regardless, there are lots of good changes and other additions in this PR. Thanks for all your work!

Comment thread alembic/README

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe this should be removed, or made to point to ALEMBIC.md

Comment thread config/events.toml
name = "FSGP_2024_Day_1"
vehicle = "Brightside"
description = "First day of FSGP 2024 at NCM Motorsports"
stages = ["Power", "Efficiency"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The stages for FSGP 2024 day 1 should match what's currently in realtime for this to work.

Comment thread docker-compose.yaml
ports:
- "5432:5432"
volumes:
- ./data/postgresql:/var/lib/postgresql/data

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know it's not orchestration related but can we change this to - pgdata:/var/lib/postgresql/data? It makes running on WSL easier.

Comment thread docker-compose.yaml

volumes:
telemetry_db_data:
telemetry_db_data:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add: pgdata:

@joshuaRiefman

Copy link
Copy Markdown
Member Author

Committing API tokens feels strange even though it's safe in our case...

Agreed. I could remove them but then we'd have to deal with .env files which is just frankly a pain (maybe necessary though).

Agreed that it is definitely more complex than what I set out to do, but I've been struggling to find places to reduce the complexity and I'd be happy to follow through with any suggestions.

since we will probably not be running offline Sunbeam very often

I used to think this too, but my opinion has flipped. We'll be using offline Sunbeam quite often in the process of developing Sunbeam and especially for any future data analysis.

@jonahjlee

Copy link
Copy Markdown
Contributor

since we will probably not be running offline Sunbeam very often

I used to think this too, but my opinion has flipped. We'll be using offline Sunbeam quite often in the process of developing Sunbeam and especially for any future data analysis.

Good points! I suppose changes to the derivation of values or additions of new ones would mean we would want to rerun the historical pipeline.

Agreed that it is definitely more complex than what I set out to do, but I've been struggling to find places to reduce the complexity and I'd be happy to follow through with any suggestions.

I'll keep this in mind - maybe I'll have better ideas when I've worked more with the repo hands-on.

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.

2 participants