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

Decision Worker #187

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Decision Worker #187

wants to merge 21 commits into from

Conversation

HermanG05
Copy link

Created the decision worker in modules/decision

Modified Files:

  • decision_worker.py
  • flight_interface_worker.py

Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

modules/decision/decision_worker.py Outdated Show resolved Hide resolved
modules/decision/decision_worker.py Outdated Show resolved Hide resolved
modules/decision/decision_worker.py Outdated Show resolved Hide resolved
modules/decision/decision_worker.py Outdated Show resolved Hide resolved
modules/decision/decision_worker.py Outdated Show resolved Hide resolved
modules/decision/decision_worker.py Outdated Show resolved Hide resolved
modules/decision/decision_worker.py Outdated Show resolved Hide resolved
modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed. Make sure to also write an integration test for the new worker.

Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
modules/decision/decision_worker.py Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

Comment on lines 42 to 50
result, estimator = cluster_estimation.ClusterEstimation.create(
min_activation_threshold, min_new_points_to_run, random_state
)
assert result
assert estimator is not None

result, cluster_pads = estimator.run(input_queue, False)
assert result
assert cluster_pads is not None
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I run the test, it crashes here. However, that's less important.

Instead of instantiating the cluster estimation object, create a fake list of object in world that is placed into the queue, similar to simulate_flight_interface_worker() below. If you want to be extra fancy, you can optionally make a list of these so that they can be selected in the loop.

Test by running: python -m tests.integration.test_decision_worker

Copy link
Author

Choose a reason for hiding this comment

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

What error are you getting within the cluster estimation function? When I try to run the test, I get a "ModuleNotFoundError".

Copy link
Collaborator

Choose a reason for hiding this comment

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

 if len(detections) == 0:
TypeError: object of type 'QueueProxyWrapper' has no len()

Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

Comment on lines 37 to 60
# Initalize queue with a maximum size of 1 to only hold latest odometry data
odometry_queue = queue_proxy_wrapper.QueueProxyWrapper(mp.Manager(), maxsize=1)

Copy link
Collaborator

@Xierumeng Xierumeng Jul 31, 2024

Choose a reason for hiding this comment

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

Wait, why is this being created here? This queue should be passed in.

Add a check that the maximum size of the queue is 1 above line 32.

Update test_flight_interface_worker.py and main_2024.py as the interface will change.

tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
tests/integration/test_decision_worker.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

main_2024.py Outdated Show resolved Hide resolved
main_2024.py Outdated Show resolved Hide resolved
modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
modules/flight_interface/flight_interface_worker.py Outdated Show resolved Hide resolved
tests/integration/test_flight_interface_worker.py Outdated Show resolved Hide resolved
tests/integration/test_flight_interface_worker.py Outdated Show resolved Hide resolved
@HermanG05 HermanG05 force-pushed the test-branch branch 2 times, most recently from 6b1c2d4 to ae44e9e Compare August 3, 2024 01:00
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.

3 participants