Implementing cleanup of NodeODM tasks

[allisonsibrian]

What type of PR is this? (check all applicable)

• 🍕 Feature
• 🐛 Bug Fix
• 📝 Documentation
• 🧑‍💻 Refactor
• ✅ Test
• 🤖 Build or CI
• ❓ Other (please specify)

Related Issue

Adds: Cleanup NodeODM job after processing is complete #641

Describe this PR

This PR implements the automatic cleanup on the NodeODM server after the processing job is completed or fails.

I added a call to task.remove() within the finally block of the process_assets_from_odm function for the cleanup process.

Alternative Approaches Considered

I attempted different ways to test the changes locally doing end-to-end testing, since it's easier to track logs, but encountered some environment configuration issues:

• When using the local UI to test: the front-end login wasn't functional due to the dummy credentials.
• Using the NodeODM Web UI for callback: the callback request from the NodeODM container was blocked with a 403 Forbidden error, which prevented my log messages from appearing in the backend logs to actually check if cleanup process occurred.

I'm unable to confirm changes because of these issues, and am just creating this as a draft PR to see if the CI tests work or to solicit advice on addressing the test issues above.

Review Guide

I feel like end-to-end testing seems like the right approach to confirm these changes (let me know if I am mistaken or if an alternate testing method is better in this case):

• Trigger a NodeODM processing job.
• After the job finishes, monitor the backend logs for confirmation messages that were added:
  - Attempting to delete task {odm_task_id} from NodeODM.
  - Successful attempt at deleting task {odm_task_id} from NodeODM.
  -Verify the task no longer appears in the NodeODM UI list.

Checklist before requesting a review

• 📖 Read the HOT Code of Conduct: https://docs.hotosm.org/code-of-conduct
• 👷‍♀️ Create small PRs. In most cases, this will be possible.
• ✅ Provide tests for your changes.
• 📝 Use descriptive commit messages.
• 📗 Update any related documentation and include any relevant screenshots.

[optional] What gif best describes this PR or how it makes you feel?

[spwoodcock]

Looks like a pretty reasonable approach to me!

Two ways of testing:
1 Connect an ODM instance and run the processing manually. View the log outputs etc to confirm.
2. e2e tests would be excellent, as you mention, but I'm not sure we have this all in place, and is probably outside the scope of this PR. If you have time to dabble with an e2e test setup (perhaps using playwright), then feel free to try! 🙏

[allisonsibrian]

Two ways of testing: 1 Connect an ODM instance and run the processing manually. View the log outputs etc to confirm.

So I tried this method,
I wasn't able to confirm/see final log outputs for the cleanup step since the backend failed to connect to NodeODM.
Debugging showed that there's a mismatch between the API path by the backend's library and the actual path used by the NodeODM server, resulting in 404 Not Found errors. I'm not sure how to address this issue.

To me, I feel like the code logic makes sense, but it's just the testing configuration preventing seeing the log outputs.
So I'm not sure if I can mark this as ready for review or if you advise to keep troubleshooting to confirm the log messages to be 100% sure it works.

[allisonsibrian]

To me, I feel like the code logic makes sense, but it's just the testing configuration preventing seeing the log outputs. So I'm not sure if I can mark this as ready for review or if you advise to keep troubleshooting to confirm the log messages to be 100% sure it works.

Just wanted to bump this message

[spwoodcock]

Apologies - I thought I would have time to test this myself, but under too much time pressure.

We need to test locally, as a big change was just merged to dev that we probably can't afford to mix with this right now.

The key is that there are no NodeODM instances attached to the default compose config.

If you wish to test the processing, I would suggest adding a nodeodm container as part of compose.test.yaml and then updating your env to use the compose service name

http://nodeodm:9000 (I think port 9000 is the default from memory)

[allisonsibrian]

Apologies - I thought I would have time to test this myself, but under too much time pressure.

We need to test locally, as a big change was just merged to dev that we probably can't afford to mix with this right now.

The key is that there are no NodeODM instances attached to the default compose config.

If you wish to test the processing, I would suggest adding a nodeodm container as part of compose.test.yaml and then updating your env to use the compose service name

http://nodeodm:9000 (I think port 9000 is the default from memory)

No worries, thank you for clarifying! I'll have time later this week to work on this again and will follow your suggestions on testing, and provide updates when I do so!

[allisonsibrian]

Just wanted to add a quick update since it's been a while:
I'm still working with this! and am making progress on the local testing setup, although it's moving a bit slowly right now.
I'm currently working on resolving some frontend and backend communication issues that are blocking me from testing the cleanup change.
I'm working on it whenever I have some free time and hoping once I have my environment sorted I can provide a better update!

[spwoodcock]

Thank you @allisonsibrian !!