dragonejt commented 5 months ago

Description

Updates the config .ini file specification format used by OnAIR to follow the requirements of #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 #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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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

dragonejt commented 4 months ago

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

dragonejt commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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: https://github.com/nasa/OnAIR/pull/98

dragonejt commented 2 months ago

A no-formatting revision has been pushed.

dragonejt commented 2 months ago

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

dragonejt commented 2 months ago

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

dragonejt commented 2 months ago

@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.

dragonejt commented 2 months ago

@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.

dragonejt commented 1 month ago

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.

nasa / OnAIR

Update config file specification format #128

Description

Testing

Considerations

Rev 2

Testing