Update config file specification format #128

dragonejt · 2024-03-23T02:08:28Z

Description

Updates the config .ini file specification format used by OnAIR to follow the requirements of Update section and key names in config files for clarity and code usage #101
Updates execution engine config file parser to read correct keys
Updates execution enginer unit tests to provide valid fake data

Testing

python driver.py ran with no errors
python driver.py -t (all unit tests) succeeded

Considerations

I had used VSCode's autoformatter to format test_execution_engine.py when updating the fake data to verify the fake data was nested correctly. If we want to go back to the original format for some reason I can figure something out to revert the formatting, but as an action item I can also take a look at adding a linter and autoformatter if we want to ensure a consistent code style.
I had posted a comment in Update section and key names in config files for clarity and code usage #101 about it being difficult to find any references for the .ini file specification. Has it been considered to move to a more modern/commonly used python configuration file format such as TOML (which visually appears very similar to .ini)? Python has a built-in library for reading TOML files as well, and modern python libraries use a pyproject.toml file for configurations.
EDIT: I have not been able to make the OPTIONS section optional, will modify functionality in the next commit

dragonejt · 2024-03-31T19:30:21Z

@the-other-james @asgibson just in case y'all haven't seen this PR, I updated the config file specification format to what was wanted in #128

asgibson · 2024-04-01T12:54:41Z

@dragonejt at a glance your coding looks very good, but my formal review will take a little more time. After my review, it will need a secondary review before a merge. There are two reviews ahead in the pipeline #125 and #127 (#98 is on hold).

In your comment, you mention #128, but as this comment is in 128, I assume you mean the issue #101. In my cursory review you appear to be hitting all the marks on the requests in that issue. While awaiting the formal review, are you able to create a feature branch for #101 and rebase it to branch 126? We like to use branches for issue tracking and the rebase will make merging smoother after the other PRs are complete.

Thank you very much for your time and effort, it is appreciated!

dragonejt · 2024-04-01T14:37:32Z

Sorry, yes I meant that this PR addresses #101 . Unfortunately, as I am not a collaborator on this repository I cannot create new branches on the repo or push to any branch on the repo. Instead, I forked the repository and submitted the PR from the fork to the original, as that was the guidance from reading this. According to that doc, there is the option to be added as a collaborator to the repository on a case-by-case basis if that is something that we want to pursue. Otherwise, the best I can do is currently what is being done, which is forking the repo and submitting the PR from my fork onto the original repository.

asgibson · 2024-04-01T14:55:37Z

No worries, I thought that may be the case. Thank you for looking into it.

the-other-james

Great work! And thank you for your patience with us!

Before I approve, please update the branch you are merging in to have the latest from our main. I think there are likely conflicts with the redis .ini file since Alan had made updates.

Also, please address my annoying nitpicks about formatting :)

Thanks!

.gitignore

onair/config/default_config.ini

onair/src/run_scripts/execution_engine.py

asgibson

Good work! All 8 of the requested changes in #101 are addressed.
Some minor changes to make, but overall looks great.

onair/config/default_config.ini

onair/config/reporter_config.ini

onair/config/redis_example.ini

onair/config/default_config.ini

onair/src/run_scripts/execution_engine.py

test/onair/src/run_scripts/test_execution_engine.py

dragonejt · 2024-04-28T22:14:15Z

Sorry for the delay, I've been busy. I'll work on these changes over the next few weeks and submit a revision.

dragonejt · 2024-06-15T17:52:37Z

Hey sorry I haven't gotten to revising this PR, I'm going to address the comments here over the next week or so. Are there any updates to how we use the .ini file that need to be reflected in this PR? Also, looking at the formatting changes requested, I was thinking about bringing in a Python code formatter so that I didn't need to manually format the codebase, such as black or Ruff. Is there any reason I should not use an autoformatter?

dragonejt · 2024-06-18T03:58:59Z

Rev 2

Remove Dev_enabled and Viz_enabled feature flags
Replace deprecated distutils.copy_tree with new shutil.copytree
Add all items in GitHub's default python .gitignore with a newline at the bottom. This covers all files previously ignored, and more files that we should probably ignore.
Updated a unit test failure by changing how a class instance was created, please let me know if we need to change it back

FAILED test/onair/src/run_scripts/test_execution_engine.py::test_ExecutionEngine_parse_configs_skips_OPTIONS_when_the_required_section_OPTIONS_is_not_in_config - AttributeError: 'ExecutionEngine' object has no attribute 'IO_Enabled'

I believe __new__ doesn't actually call __init__, so changing it to how normal class instances are created calls __init__.

Testing

All Unit Tests succeeded.

the-other-james · 2024-06-20T17:59:53Z

Could you please remove the changes related to formatting? I don't want to mix formatting changes in with your other changes: it makes for a huge pull request (62 changed files) which obfuscates the config file update.

Reformatting of the entire code base should be done as its own pull request. Alan started looking into this a while ago: #98

…tignore, remove multiline f-string

dragonejt · 2024-06-21T02:33:22Z

A no-formatting revision has been pushed.

dragonejt · 2024-06-27T23:21:18Z

Just in case you two haven't seen this, @the-other-james @asgibson

the-other-james

Looks good to me! Tested default config and unit tests on my machine.

the-other-james

I did a bit more testing and noticed that the kalman and redis configs hadn't been fully updated. I'm not sure if I can directly push my fixes to your merge request, so here's the diff that got things working on my end:

diff --git a/onair/config/kalman_csv_output_example.ini b/onair/config/kalman_csv_output_example.ini
index 06dab42..a1c927d 100644
--- a/onair/config/kalman_csv_output_example.ini
+++ b/onair/config/kalman_csv_output_example.ini
@@ -1,14 +1,17 @@
-[DEFAULT]
-TelemetryDataFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
+[FILES]
+TelemetryFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
 TelemetryFile = 700_crash_to_earth_1.csv
-TelemetryMetadataFilePath = onair/data/telemetry_configs/
+MetaFilePath = onair/data/telemetry_configs/
 MetaFile = data_physics_generation_CONFIG.json
-ParserFileName = onair/data_handling/csv_parser.py

+[DATA_HANDLING]
+DataSourceFile = onair/data_handling/csv_parser.py
+
+[PLUGINS]
 KnowledgeRepPluginDict = {'Kalman Filter': 'plugins/kalman'}
 LearnersPluginDict = {'csv output':'plugins/csv_output'}
 PlannersPluginDict = {}
 ComplexPluginDict = {}

-[RUN_FLAGS]
-IO_Flag = true
+[OPTIONS]
+IO_Enabled = true
diff --git a/onair/config/redis_example.ini b/onair/config/redis_example.ini
index b0a599b..3f70e42 100644
--- a/onair/config/redis_example.ini
+++ b/onair/config/redis_example.ini
@@ -3,7 +3,9 @@ TelemetryFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
 TelemetryFile = 700_crash_to_earth_1.csv
 MetaFilePath = onair/data/telemetry_configs/
 MetaFile = redis_example_CONFIG.json
-ParserFileName = onair/data_handling/redis_adapter.py
+
+[DATA_HANDLING]
+DataSourceFile = onair/data_handling/redis_adapter.py

 [PLUGINS]
 KnowledgeRepPluginDict = {'knowledge':'plugins/generic/__init__.py'}

dragonejt · 2024-07-10T16:59:14Z

I did a bit more testing and noticed that the kalman and redis configs hadn't been fully updated. I'm not sure if I can directly push my fixes to your merge request, so here's the diff that got things working on my end:

diff --git a/onair/config/kalman_csv_output_example.ini b/onair/config/kalman_csv_output_example.ini
index 06dab42..a1c927d 100644
--- a/onair/config/kalman_csv_output_example.ini
+++ b/onair/config/kalman_csv_output_example.ini
@@ -1,14 +1,17 @@
-[DEFAULT]
-TelemetryDataFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
+[FILES]
+TelemetryFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
 TelemetryFile = 700_crash_to_earth_1.csv
-TelemetryMetadataFilePath = onair/data/telemetry_configs/
+MetaFilePath = onair/data/telemetry_configs/
 MetaFile = data_physics_generation_CONFIG.json
-ParserFileName = onair/data_handling/csv_parser.py

+[DATA_HANDLING]
+DataSourceFile = onair/data_handling/csv_parser.py
+
+[PLUGINS]
 KnowledgeRepPluginDict = {'Kalman Filter': 'plugins/kalman'}
 LearnersPluginDict = {'csv output':'plugins/csv_output'}
 PlannersPluginDict = {}
 ComplexPluginDict = {}

-[RUN_FLAGS]
-IO_Flag = true
+[OPTIONS]
+IO_Enabled = true
diff --git a/onair/config/redis_example.ini b/onair/config/redis_example.ini
index b0a599b..3f70e42 100644
--- a/onair/config/redis_example.ini
+++ b/onair/config/redis_example.ini
@@ -3,7 +3,9 @@ TelemetryFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
 TelemetryFile = 700_crash_to_earth_1.csv
 MetaFilePath = onair/data/telemetry_configs/
 MetaFile = redis_example_CONFIG.json
-ParserFileName = onair/data_handling/redis_adapter.py
+
+[DATA_HANDLING]
+DataSourceFile = onair/data_handling/redis_adapter.py

 [PLUGINS]
 KnowledgeRepPluginDict = {'knowledge':'plugins/generic/__init__.py'}

Ah, I may have not fully changed the other configs, sorry about that. I'll get that fixed

…flect changes

dragonejt · 2024-07-10T17:53:31Z

@the-other-james It looks like what happened was that I did indeed miss the error in redis_example.ini, but the commit that added onair/config/kalman_csv_output_example.ini took place after I created my fork from the main repo, so my fork didn't have that file. The new commit I just pushed has the updates, and should work now.

asgibson

Please revert the ExecutionEngine instantiations in the unit tests. By only allocating space for the instances it makes the tests more maintainable because changes in the __init__ function will not affect the tests (as it will not run). This also requires the tests to explicitly set any attributes within the test itself because initialization was not performed on the instance, which provides more information about how the code under test (cut) operates.

test/onair/src/run_scripts/test_execution_engine.py

.gitignore

dragonejt · 2024-07-14T18:29:19Z

@asgibson the ExecutionEngine issue has been fixed, and I left a comment about where I got my Python .gitignore from. Please take a look when you have time.

the-other-james

Great work! Thank you for your patience!

dragonejt · 2024-07-26T19:05:42Z

Hello, I don't have permissions to merge into the main branch, so one of you will need to merge whenever we're ready to merge.

dragonejt added 2 commits March 22, 2024 21:53

Update config file specification format

bc2322f

enable OPTIONS section to be optional

c301ae5

the-other-james requested changes Apr 19, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

onair/config/default_config.ini Show resolved Hide resolved

onair/src/run_scripts/execution_engine.py Show resolved Hide resolved

asgibson requested changes Apr 19, 2024

View reviewed changes

dragonejt added 2 commits May 2, 2024 21:35

Merge branch 'main' of https://github.com/nasa/OnAIR into nasa-main

1dba643

Merge branch 'nasa-main'

0ca55e7

fix: Remove dev_enabled and viz_enabled, use GitHub default python gi…

9eb12bd

…tignore, remove multiline f-string

dragonejt force-pushed the main branch from 895cd0b to 9eb12bd Compare June 21, 2024 02:31

the-other-james approved these changes Jul 10, 2024

View reviewed changes

the-other-james requested changes Jul 10, 2024

View reviewed changes

dragonejt added 2 commits July 10, 2024 13:08

Merge branch 'nasa:main' into main

34b0ba8

fix: Update redis_example.ini and kalman_csv_output_example.ini to re…

b8e3f15

…flect changes

asgibson requested changes Jul 11, 2024

View reviewed changes

test/onair/src/run_scripts/test_execution_engine.py Outdated Show resolved Hide resolved

.gitignore Show resolved Hide resolved

dragonejt added 2 commits July 11, 2024 17:50

fix: revert ExecutionEngine instantiation in tests to use __new__

1791346

fix: re-add .gitignore to ignore macOS .DS_Store files

c8f0bab

asgibson approved these changes Jul 26, 2024

View reviewed changes

the-other-james approved these changes Jul 26, 2024

View reviewed changes

Merge branch 'main' into main

74c983d

asgibson merged commit c59d04d into nasa:main Jul 26, 2024
5 checks passed

the-other-james mentioned this pull request Jul 29, 2024

kalman/csv using Redis? Should use a csv file. #142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update config file specification format #128

Update config file specification format #128

dragonejt commented Mar 23, 2024 •

edited

Loading

dragonejt commented Mar 31, 2024

asgibson commented Apr 1, 2024

dragonejt commented Apr 1, 2024 •

edited

Loading

asgibson commented Apr 1, 2024

the-other-james left a comment

asgibson left a comment

dragonejt commented Apr 28, 2024

dragonejt commented Jun 15, 2024

dragonejt commented Jun 18, 2024 •

edited

Loading

the-other-james commented Jun 20, 2024

dragonejt commented Jun 21, 2024

dragonejt commented Jun 27, 2024

the-other-james left a comment

the-other-james left a comment

dragonejt commented Jul 10, 2024

dragonejt commented Jul 10, 2024

asgibson left a comment

dragonejt commented Jul 14, 2024

the-other-james left a comment •

edited

Loading

dragonejt commented Jul 26, 2024

Update config file specification format #128

Update config file specification format #128

Conversation

dragonejt commented Mar 23, 2024 • edited Loading

Description

Testing

Considerations

dragonejt commented Mar 31, 2024

asgibson commented Apr 1, 2024

dragonejt commented Apr 1, 2024 • edited Loading

asgibson commented Apr 1, 2024

the-other-james left a comment

Choose a reason for hiding this comment

asgibson left a comment

Choose a reason for hiding this comment

dragonejt commented Apr 28, 2024

dragonejt commented Jun 15, 2024

dragonejt commented Jun 18, 2024 • edited Loading

Rev 2

Testing

the-other-james commented Jun 20, 2024

dragonejt commented Jun 21, 2024

dragonejt commented Jun 27, 2024

the-other-james left a comment

Choose a reason for hiding this comment

the-other-james left a comment

Choose a reason for hiding this comment

dragonejt commented Jul 10, 2024

dragonejt commented Jul 10, 2024

asgibson left a comment

Choose a reason for hiding this comment

dragonejt commented Jul 14, 2024

the-other-james left a comment • edited Loading

Choose a reason for hiding this comment

dragonejt commented Jul 26, 2024

dragonejt commented Mar 23, 2024 •

edited

Loading

dragonejt commented Apr 1, 2024 •

edited

Loading

dragonejt commented Jun 18, 2024 •

edited

Loading

the-other-james left a comment •

edited

Loading