Skip to content

Conversation

@machshev
Copy link
Collaborator

@machshev machshev commented Oct 9, 2025

Decouple the Deploy objects a little bit more from the rest of the system.

Python objects are shared by using reference counting, and the object will not be garbage collected until all references are dropped. Best practice is to avoid storing references unless there is a good reason to, the more references that are left around the more likely to encounter memory issues.

The other issue is logical coupling. Objects like sim_cfg and the cli args Namespace object are passed all the way through the whole flow and downstream systems pull out attributes that they want. Which means that any changes in these objects can break any downstream systems, making it really difficult to refactor anything.

This PR improves this a little bit by pulling back some of those references.

  • The schedule run results dict is migrated from Mapping[Deploy, str] -> Mapping[str,CompletedJobStatus] - in later refactoring this may be improved further, however this means the results are keyed by the name of the deployment objects and not the Deploy objects themselves.
  • Launcher objects were being stored in the Deploy objects as public attributes for a single callback function to use. Instead of doing that, we now pass the launcher object to the callback only when required. With further refactoring we might not even need to do that.
  • The Scheduler is passed the Launcher class object and then each Deploy object is asked to create it's own launcher object. From what I can see the only reason is so that it can be stored for the callback. This is now unnecessary so the Launcher objects can be constructed directly.
  • The Scheduler.run() method returned a mapping of Deploy objects to status strings. But then the results generation functions were accessing further fields from the Launcher via the Deploy object. Instead of doing this the Scheduler now returns a CompletedJobStatus object that contains all the fields required to begin with.

Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

A couple of small comments (since you're going to have to force-push anyway :-) ). Thanks for working to decouple this a bit: it's definitely going in the right direction!

@machshev machshev force-pushed the remove-sim-cfg-from-deploy branch 2 times, most recently from 25d4686 to 49ad24c Compare October 10, 2025 10:51
@machshev machshev force-pushed the remove-sim-cfg-from-deploy branch from 49ad24c to 4529ba7 Compare October 10, 2025 10:55
@machshev machshev requested a review from rswarbrick October 10, 2025 10:57
@machshev machshev merged commit 1102d8d into lowRISC:master Oct 10, 2025
6 checks passed
@machshev machshev deleted the remove-sim-cfg-from-deploy branch October 10, 2025 13:17
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