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

Remove task wise mode #699

Merged
merged 3 commits into from
Jan 29, 2025
Merged

Remove task wise mode #699

merged 3 commits into from
Jan 29, 2025

Conversation

oysand
Copy link
Contributor

@oysand oysand commented Jan 24, 2025

Ready for review checklist:

  • A self-review has been performed
  • All commits run individually
  • Temporary changes have been removed, like logging, TODO, etc.
  • The PR has been tested locally
  • A test has been written
    • This change doesn't need a new test
  • Relevant issues are linked
  • Remaining work is documented in issues
    • There is no remaining work from this PR that requires new issues
  • The changes do not introduce dead code as unused imports, functions etc.

@Eddasol
Copy link
Contributor

Eddasol commented Jan 27, 2025

  • There are a lot of taskwise related comments in robot_interface that can be removed as well. Like this one:
This function should be used in combination with the mission_status function
if the robot is designed to run full missions and not in a taskwise
configuration.
  • Initiate_task in robot_interface is only needed for task wise missions and can be removed

@oysand oysand force-pushed the remove-task-wise branch 3 times, most recently from c01665c to daff8a8 Compare January 28, 2025 10:46
@oysand oysand requested a review from Christdej January 28, 2025 10:53
@@ -71,7 +61,7 @@ def initiate_task(self, task: Task) -> None:
of the task until the number of allowed retries is exceeded before the task
will be marked as failed and the mission cancelled
NotImplementedError
Copy link
Contributor

Choose a reason for hiding this comment

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

A few lines above this function still says "RobotInfeasibleTaskException" which I believe is no longer possible? Also you can delete this exception from robot_exception.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is still a reasonable exception as we want to keep initiate_task for scheduling retrun to home from isar directly

Copy link
Contributor

Choose a reason for hiding this comment

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

So we want to initiate it as a task and not as a mission?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would argue so, yes

@oysand oysand requested a review from Christdej January 28, 2025 11:21
@andchiind
Copy link
Contributor

It might be easier to merge this PR first: #689 since it isolates all the task-wise code into specific transitions. Instead of deleting specific lines in various states, all we would need to do is delete the relevant transitions (and remove the Task option in starting missions).

Copy link
Contributor

@Christdej Christdej left a comment

Choose a reason for hiding this comment

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

LGTM

@oysand
Copy link
Contributor Author

oysand commented Jan 28, 2025

It might be easier to merge this PR first: #689 since it isolates all the task-wise code into specific transitions. Instead of deleting specific lines in various states, all we would need to do is delete the relevant transitions (and remove the Task option in starting missions).

As this change entails less change in isar, we will take this in first and continue with it afterwards

@oysand oysand merged commit 246f355 into equinor:main Jan 29, 2025
7 checks passed
@oysand oysand deleted the remove-task-wise branch January 29, 2025 07:28
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.

4 participants