search

Lilypad-Tech / lilypad

Run AI workloads easily in a decentralized GPU network. https://www.youtube.com/watch?v=yQnB2Yxia4Y
https://lilypad.tech
Apache License 2.0
52 stars 16 forks source link

chore: Remove CommandContext.CommandContext #410

Closed bgins closed 4 weeks ago

bgins commented 4 weeks ago

Summary

This pull request makes the following changes:

We set up the command context with a CommandContext.CommandContext and a CommandContext.Ctx, but they are the same context:

https://github.com/Lilypad-Tech/lilypad/blob/b4e56e34a4f6b6b3979e9523731b8952162ad143/pkg/system/context.go#L25-L31

CommandContext.CommandContext is not referenced anywhere outside the CommandContext type, the set up function, and a clean up call. It seems safe to remove.

Task/Issue reference

Closes: https://github.com/Lilypad-Tech/internal/issues/296

Test plan

Run a job. The job should complete successfully.

When services are shut down after the job, they should no longer emit a CleanupManager: Cleanup called again after already called warning.

Details

We initially noticed this issue when we started seeing CleanupManager: Cleanup called again after already called warnings. Cleanup was called on (effectively) the same context, and the same set of functions.

The cleanup manager design isn't entirely clear. We think that functions could be associated with their contexts, and then each cleanup function could be run with its context. The current design does not support this approach.

We may want to revisit in the future if we decide we would like more than one context.

walkah commented 4 weeks ago

Closes: https://github.com/Lilypad-Tech/internal/issues/296

bgins commented 4 weeks ago

Can we just remove CommandContext.CommandContext altogether? It looks to me like it's never used. (remove it from the strct and in NewSystemContext just don't set it).

Yeah, good call! Removed it and updated the top-level PR description.