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

Add missing nodes to various places in InferenceSystem #151

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

dthaler
Copy link
Collaborator

@dthaler dthaler commented Jul 22, 2024

Eventually these should all pull from https://live.orcasound.net/api/json/feeds but the first step is to try adding the missing ones manually.

@dthaler dthaler force-pushed the nodes branch 4 times, most recently from 982d284 to 800ecd9 Compare July 22, 2024 21:33
@dthaler dthaler requested review from pastorep and micya July 22, 2024 21:35
Copy link
Member

@micya micya left a comment

Choose a reason for hiding this comment

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

"Update the last line of the Dockerfile to point to the new config file"

In theory, each container should have its own dockerfile. In practice, the original owners used one dockerfile and manually changed the last line before executing docker build.

I do not know how to test this PR.

You should test the individual docker images by running docker run locally and verifying that the logs look as expected (does not crash).

I am not sure if the github CI/CD workflows exercise these scripts or not.

No CI/CD is currently setup for the inference system. We manually deploy to kubernetes using the instructions here.

Although we no longer deploy on Azure Container Instances, it is good practice to keep the ACI deployment config up to date (as you have done in this PR) in the unlikely chance that Azure fixes the stability issues and we move back to ACI.

InferenceSystem/deploy/mast-center.yaml Outdated Show resolved Hide resolved
@dthaler
Copy link
Collaborator Author

dthaler commented Jul 24, 2024

"Update the last line of the Dockerfile to point to the new config file"

In theory, each container should have its own dockerfile. In practice, the original owners used one dockerfile and manually changed the last line before executing docker build.

What's confusing in the README is that it implies that the PR should update the Dockerfile. But it sounds like that's not the case. It sounds like the README should be updated to say to not check in the Dockerfile changes, but to do it locally, once for each hydrophone being added, etc. per the explanation you gave in comments above.

@micya
Copy link
Member

micya commented Jul 24, 2024

"Update the last line of the Dockerfile to point to the new config file"

In theory, each container should have its own dockerfile. In practice, the original owners used one dockerfile and manually changed the last line before executing docker build.

What's confusing in the README is that it implies that the PR should update the Dockerfile. But it sounds like that's not the case. It sounds like the README should be updated to say to not check in the Dockerfile changes, but to do it locally, once for each hydrophone being added, etc. per the explanation you gave in comments above.

Your understanding is correct. The proper fix is to add a dockerfile per location.

@dthaler
Copy link
Collaborator Author

dthaler commented Aug 7, 2024

You should test the individual docker images by running docker run locally

I tried following the instructions to do so but couldn't even get far enough to try. Filed issue #156 which is blocking my ability to test locally.

@micya
Copy link
Member

micya commented Sep 16, 2024

I will approve once this PR is tested as noted in #151 (comment).

@dthaler
Copy link
Collaborator Author

dthaler commented Sep 17, 2024

I will approve once this PR is tested as noted in #151 (comment).

Testing is currently blocked on #156

@dthaler dthaler added the inference system Code to perform inference with the trained model(s) label Sep 17, 2024
@micya
Copy link
Member

micya commented Sep 19, 2024

Obsoleted by #197.

@dthaler
Copy link
Collaborator Author

dthaler commented Sep 19, 2024

Obsoleted by #197.

Only partially. There are some things in this PR that are not in 197. I will rebase.

@dthaler
Copy link
Collaborator Author

dthaler commented Sep 20, 2024

Obsoleted by #197.

Only partially. There are some things in this PR that are not in 197. I will rebase.

I rebased. @micya Please re-review.

  • Updated README.md so that line 179 need not be modified every time we add a hydrophone
  • Updated hls_reader.py to add the commas since the checked-in file is invalid syntax (as reported by CodeQL) and to add the remaining streams
  • Updated LiveInferenceOrchestrator.py to alphabetize the list, and also to correct the "name" of Orcasound Lab per issue Haro Strait vs Straight #145
  • Updated PrepareDataForPredictionExplorer.py to add the missing steams
  • Updated globals.py to alphabetize the list

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
Copy link
Member

@micya micya left a comment

Choose a reason for hiding this comment

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

No behavior change. Redeployment not required.

@dthaler dthaler merged commit 608b5a0 into orcasound:main Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inference system Code to perform inference with the trained model(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants