Skip to content

Various fixes to CLEM workflow and bootstrap API endpoints#494

Merged
tieneupin merged 19 commits intomainfrom
feedback-queue-fix
Feb 20, 2025
Merged

Various fixes to CLEM workflow and bootstrap API endpoints#494
tieneupin merged 19 commits intomainfrom
feedback-queue-fix

Conversation

@tieneupin
Copy link
Contributor

@tieneupin tieneupin commented Feb 13, 2025

One thing led to another, and now there are a couple of fixes present in this PR:

  • Loaded 'feedback_queue' from either the security config file or the TransportManager object. Resolves issue Some functions still look for 'feedback_queue' in MachineConfig #487 .
  • Added unit tests for some of the functions fixed as part of this PR.
  • Fixed some race conditions uncovered when refreshing database entries for the CLEM workflow.
  • More verbose logging for CLEM workflow.
  • Corrected canonical representation of variables in log messages for "bootstrap" API endpoints

Checks:

  • CLEM workflow works on test setup

@tieneupin tieneupin added bug Something isn't working server Relates to the server component labels Feb 13, 2025
@tieneupin tieneupin self-assigned this Feb 13, 2025
@tieneupin tieneupin linked an issue Feb 13, 2025 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 6.77966% with 55 lines in your changes missing coverage. Please review.

Project coverage is 28.61%. Comparing base (400765c) to head (5acdbff).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
+ Coverage   27.93%   28.61%   +0.67%     
==========================================
  Files          78       78              
  Lines       10255    10218      -37     
  Branches     1355     1354       -1     
==========================================
+ Hits         2865     2924      +59     
+ Misses       7295     7194     -101     
- Partials       95      100       +5     

@tieneupin tieneupin changed the title Get 'feedback_queue' from either the security config file or the TransportObject class Get 'feedback_queue' from either the security config file or the TransportManager object Feb 13, 2025
@tieneupin tieneupin marked this pull request as ready for review February 13, 2025 11:40
@tieneupin tieneupin changed the title Get 'feedback_queue' from either the security config file or the TransportManager object Various fixes to CLEM workflow and bootstrap API endpoints Feb 19, 2025
Copy link
Contributor

@stephen-riggs stephen-riggs left a comment

Choose a reason for hiding this comment

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

Mostly looks fine. I'm a little unsure on the bits avoiding race conditions though. It would be nicer if you didn't have to do the waits, so is there a way to avoid doing the checks and refreshes?
I've noted the places you do the waits. If they're unavoidable then just dismiss the comments, but it feels like some of the checks aren't needed.

@tieneupin
Copy link
Contributor Author

Mostly looks fine. I'm a little unsure on the bits avoiding race conditions though. It would be nicer if you didn't have to do the waits, so is there a way to avoid doing the checks and refreshes? I've noted the places you do the waits. If they're unavoidable then just dismiss the comments, but it feels like some of the checks aren't needed.

Your suggestion worked! I disabled all the db.refresh() entries and ran the workflow again, and it seems to be progressing with no failures. Looks like the refreshes were indeed unnecessary.

Your suggestion to remove the is_file() check from the validate_and_sanitise() functions was also sound. It caused more problems than it solved, since the file might not yet exist on the server-side at the time that check is called.

Copy link
Contributor

@stephen-riggs stephen-riggs left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, it looks much cleaner without the refreshes and sleeps. Looks good to me now :)

@tieneupin tieneupin merged commit 5c94171 into main Feb 20, 2025
17 checks passed
@tieneupin tieneupin deleted the feedback-queue-fix branch February 20, 2025 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working server Relates to the server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some functions still look for 'feedback_queue' in MachineConfig

2 participants