Skip to content

feat: Connection and Upload Managers use config from Config Manager #280

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.

Sign up for GitHub

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

Jump to bottom
Open
damonhayhurst-neuraco wants to merge 1 commit into
base: ndd-dev
Choose a base branch
from
Open

feat: Connection and Upload Managers use config from Config Manager #280

damonhayhurst-neuraco wants to merge 1 commit into from

Conversation

@damonhayhurst-neuraco
Copy link

@damonhayhurst-neuraco damonhayhurst-neuraco commented Jan 21, 2026 •
edited
Loading

Features

  • Connection Manager, Upload manager use named params from config
  • Connection Manager Responds to offline mode
  • RDM utilised named config rather than resolving config
  • Suite of tests

@damonhayhurst-neuraco damonhayhurst-neuraco added the version:minor non-breaking feature updates, new functions or endpoints label Jan 21, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 21, 2026

Consider updating changelogs/pending-changelog.md with a summary of this change for the release notes. This is optional and non-blocking.

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Jan 21, 2026

Consider updating changelogs/pending-changelog.md with a summary of this change for the release notes. This is optional and non-blocking.

@damonhayhurst-neuraco damonhayhurst-neuraco changed the title feat:Connection Manager works with Config Manager and offline mode feat: Connection Manager works with Config Manager and offline mode Jan 21, 2026
@damonhayhurst-neuraco damonhayhurst-neuraco changed the title feat: Connection Manager works with Config Manager and offline mode feat:Connection Manager works with Config Manager and offline mode Jan 21, 2026
@damonhayhurst-neuraco damonhayhurst-neuraco force-pushed the feat/offline-mode-connection-mgr branch from 1337a94 to d6c9e25 Compare January 21, 2026 12:25
@damonhayhurst-neuraco damonhayhurst-neuraco changed the title feat:Connection Manager works with Config Manager and offline mode feat: Connection Manager works with Config Manager and offline mode Jan 21, 2026
@damonhayhurst-neuraco damonhayhurst-neuraco force-pushed the feat/offline-mode-connection-mgr branch 2 times, most recently from 2877541 to b9b1c58 Compare January 21, 2026 14:03
StevenJacobs61
StevenJacobs61 approved these changes Jan 21, 2026
View reviewed changes
Copy link
Contributor

@StevenJacobs61 StevenJacobs61 left a comment

Choose a reason for hiding this comment

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

LGTM

neuracore/data_daemon/connection_management/connection_manager.py

emitter.emit(Emitter.IS_CONNECTED, self._is_connected)

def start(self) -> None:
Copy link
Contributor

@StevenJacobs61 StevenJacobs61 Jan 21, 2026

Choose a reason for hiding this comment

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

We just have to remember to execute start during integration

@damonhayhurst-neuraco damonhayhurst-neuraco force-pushed the feat/offline-mode-connection-mgr branch from b9b1c58 to a3008ad Compare January 22, 2026 09:40
@damonhayhurst-neuraco damonhayhurst-neuraco changed the title feat: Connection Manager works with Config Manager and offline mode feat: Connection and Upload Managers work with Config Manager and offline mode Jan 22, 2026
StevenJacobs61
StevenJacobs61 requested changes Jan 22, 2026
View reviewed changes
Copy link
Contributor

@StevenJacobs61 StevenJacobs61 left a comment

Choose a reason for hiding this comment

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

LGTM w/c

StevenJacobs61
StevenJacobs61 approved these changes Jan 22, 2026
View reviewed changes
Copy link
Contributor

@StevenJacobs61 StevenJacobs61 left a comment

Choose a reason for hiding this comment

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

LGTM w/c

@damonhayhurst-neuraco damonhayhurst-neuraco force-pushed the feat/offline-mode-connection-mgr branch 2 times, most recently from 0245989 to c8d2150 Compare January 22, 2026 11:32
@damonhayhurst-neuraco damonhayhurst-neuraco changed the title feat: Connection and Upload Managers work with Config Manager and offline mode feat: Connection, Upload and RDM Managers use config manager Jan 22, 2026
@damonhayhurst-neuraco damonhayhurst-neuraco force-pushed the feat/offline-mode-connection-mgr branch from c8d2150 to 0be2d74 Compare January 22, 2026 11:46
@damonhayhurst-neuraco damonhayhurst-neuraco changed the title feat: Connection, Upload and RDM Managers use config manager feat: Connection, Upload and RDM Managers use config manager Jan 22, 2026
@damonhayhurst-neuraco damonhayhurst-neuraco changed the title feat: Connection, Upload and RDM Managers use config manager feat: Connection, Upload and RDM Managers use config from Config Manager Jan 22, 2026
safi-baqri-n
safi-baqri-n requested changes Jan 22, 2026
View reviewed changes
neuracore/data_daemon/connection_management/connection_manager.py
self._checker_thread: threading.Thread | None = None
self._offline_mode = offline_mode

emitter.emit(Emitter.IS_CONNECTED, self._is_connected)
Copy link
Contributor

@safi-baqri-n safi-baqri-n Jan 22, 2026

Choose a reason for hiding this comment

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

This assumes that StateManager init before ConnectionManager. This is a race condition risk. The pyee.EventEmitter is a standard pub/sub pattern - events are NOT queued. If no listener is registered when the event fires, it's lost.

We need to make sure that StateManager gets the right state at init/start, can even directly read from config, if event is lost no problem.

neuracore/data_daemon/connection_management/connection_manager.py
return
if self._running:
logger.warning("ConnectionManager already running")
return
Copy link
Contributor

@safi-baqri-n safi-baqri-n Jan 22, 2026

Choose a reason for hiding this comment

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

nit or lets note down or create a task in backlog
Since we're moving data daemon to asyncio, should this use asyncio.create_task() instead of spawning a thread.

@damonhayhurst-neuraco damonhayhurst-neuraco force-pushed the feat/offline-mode-connection-mgr branch from 0be2d74 to 65c90c6 Compare January 22, 2026 15:38
@damonhayhurst-neuraco damonhayhurst-neuraco force-pushed the feat/offline-mode-connection-mgr branch from 65c90c6 to d45c0a3 Compare January 22, 2026 15:40
@damonhayhurst-neuraco damonhayhurst-neuraco changed the title feat: Connection, Upload and RDM Managers use config from Config Manager feat: Connection and Upload Managers use config from Config Manager Jan 22, 2026
safi-baqri-n
safi-baqri-n approved these changes Jan 23, 2026
View reviewed changes
Copy link
Contributor

@safi-baqri-n safi-baqri-n left a comment

Choose a reason for hiding this comment

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

Lets consider off-line changes later, approving to enable rest of changes go though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StevenJacobs61 StevenJacobs61 StevenJacobs61 approved these changes

@safi-baqri-n safi-baqri-n safi-baqri-n approved these changes

@favour-neuraco favour-neuraco Awaiting requested review from favour-neuraco

Assignees

No one assigned

Labels

version:minor non-breaking feature updates, new functions or endpoints

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@damonhayhurst-neuraco @StevenJacobs61 @safi-baqri-n