Add a `frequency` parameter to `TriggerWandbSyncLightningCallback` #101

CedricLeon · 2024-03-22T13:20:19Z

In order to synchronize every N epoch instead of every epoch (by default), I added a frequency parameter to the Lightning callback.

I did not write any test for that feature, mainly because I am not sure how you proceed (but also because of lazyness 😅)
Tell me if you want me to add a test for that.

semanticdiff-com · 2024-03-22T13:20:22Z

Review changes with SemanticDiff.

Analyzed 3 of 3 files.

Overall, the semantic diff is 51% smaller than the GitHub diff.

	Filename	Status
✔️	src/wandb_osh/hooks.py	58.82% smaller
✔️	src/wandb_osh/lightning_hooks.py	26.87% smaller
✔️	src/wandb_osh/ray_hooks.py	74.45% smaller

klieret · 2024-03-22T16:46:01Z

src/wandb_osh/lightning_hooks.py

@@ -41,6 +43,7 @@ def __init__(
        """
        super().__init__()
        self._hook = TriggerWandbSyncHook(communication_dir=communication_dir)
+        self.frequency = frequency


could you make this private? i.e., self._frequency

Yes, good idea

klieret · 2024-03-22T16:48:24Z

I like this, this would be an alternate way of solving #83 and much simpler than implementing multiprocessing.

As for tests, I think this is fine, I'd say it's simple enough :)

klieret · 2024-03-22T21:20:05Z

src/wandb_osh/lightning_hooks.py

@@ -24,11 +24,13 @@ class TriggerWandbSyncLightningCallback(pl.Callback):
    def __init__(
        self,
        communication_dir: PathLike = _comm_default_dir,
+        frequency: int = 1,


I wonder if there's a better name for this. Because this setting is basically 1/frequency, right? (the larger this value is, the less often we sync). So perhaps something like sync_every_n_epochs?

I started with such a name but thought that frequency was shorter and still self-explanatory.
I will make the change to sync_every_n_epochs, it is more accurate

src/wandb_osh/lightning_hooks.py

…tialization Now the calling `hook()` must be done by giving information about the current epoch

…ialization and call

…tion and call

CedricLeon · 2024-03-25T13:56:34Z

I made the changes. Still without testing 😇
I am 100% confident it works for the Lightning hook (TriggerWandbSyncLightningCallback), because I use such a setup in my work. However, I never used Ray before and can't think of an easy way to test it. Can you have a look and tell me if I implemented something stupid?
Also, about #83, I think it will definitely help but won't solve the problem (I can see that, when I run big sweeps, with 20 or 40 runs in parallel 😅).

CedricLeon · 2024-03-25T14:02:34Z

I just realized that the hook() should also be called after the end of the last epoch, in case people use a weird sync_every_n_epochs. I don't think this is implemented yet.
I say this in case such thing happen:

An run running for, say, 100, epochs but sync_every_n_epochs set to 3 will miss to synchronize the last epoch.
Or (maybe more classic), a scheduler killing the run epoch 51 when sync_every_n_epochs is set to 10 or even 5 or 2. In this case a few epoch won't be uploaded.

I see an easy fix for the Lightning callback; adding the on_test_epoch_end() hook with a call to TriggerWandbSyncHook that will happen 100% of the time (e.g., by setting current-epoch = sync_every_n_epochs, that's cheesy but that will work).
But, again, I have no clue about Ray 😅

klieret · 2024-03-25T15:02:55Z

Oh you're 100% right, I should have thought about that as well. A totally different avenue of solution (that might also help with syncing hyperparameters of runs that get killed because of #100) would be that the wandb-osh syncer tool remembers which runs were syncing and if it doesn't get triggered for an hour, it runs one final sync call (and we can catch KeyboardInterrupt to make sure we also do the final round if people kill wandb-osh. Of course that's also just another silly workaround.

Let me think about that. We might in fact do both (and perhaps add a note to the ray one that it has this disadvantage).

CedricLeon · 2024-03-26T10:05:39Z

That would be a pretty robust workaround indeed.
Should I add the on_test_epoch_end() fix I mentionned and we close this PR?
Or should I just add a Warning about this problem in the doc, and we fix it in another PR?

I dug into the problem I raised in #100 and this is deeper that just not syncing the config. i will detail what I found so far in the Issue.
Anyway, it would be handy for me to have the possibility to reduce the number of Triggers to investigate when the problem happens.

CedricLeon · 2024-03-28T08:17:33Z

@klieret should I push this temporary fix?
Or do you want to solve it in this PR?

…tion on the end of the last epoch

for more information, see https://pre-commit.ci

klieret · 2024-04-01T22:28:02Z

Sorry for my late replies! Let me take a look now :)

klieret · 2024-04-01T22:54:42Z

Could you give me push permission on the branch (I made some fixes and added some tests to this). You only have to click a checkbox on this PR, I think

CedricLeon · 2024-04-04T16:41:37Z

Could you give me push permission on the branch (I made some fixes and added some tests to this). You only have to click a checkbox on this PR, I think

The box is checked, it seems to be checked by default btw

CedricLeon · 2024-04-18T13:55:29Z

Hi @klieret, any update about this PR?

Add a frequency parameter to TriggerWandbSyncLightningCallback

a8ef743

klieret requested changes Mar 22, 2024

View reviewed changes

klieret mentioned this pull request Mar 22, 2024

Wandb-osh cannot handle many runs at once #83

Open

klieret requested changes Mar 22, 2024

View reviewed changes

klieret linked an issue Mar 22, 2024 that may be closed by this pull request

Wandb-osh cannot handle many runs at once #83

Open

CedricLeon added 4 commits March 25, 2024 14:33

Renaming frequency in sync_every_n_epochs

39bc5dd

Add a sync_every_n_epochs parameter to TriggerWandbSyncHook() ini…

2882069

…tialization Now the calling `hook()` must be done by giving information about the current epoch

Updating the Lightning Hook, with the new TriggerWandbSyncHook init…

8ca3763

…ialization and call

Updating the Ray Hook, with the new TriggerWandbSyncHook initializa…

1bcd8bd

…tion and call

CedricLeon and others added 3 commits March 28, 2024 21:46

Add the on_test_epoch_end() Lightning hook to force the synchroniza…

73311ff

…tion on the end of the last epoch

[pre-commit.ci] auto fixes from pre-commit.com hooks

2d6320a

for more information, see https://pre-commit.ci

Correct stupid typo in lightning_hooks.py

6861203

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `frequency` parameter to `TriggerWandbSyncLightningCallback` #101

Add a `frequency` parameter to `TriggerWandbSyncLightningCallback` #101

CedricLeon commented Mar 22, 2024

semanticdiff-com bot commented Mar 22, 2024 •

edited

Loading

klieret Mar 22, 2024

CedricLeon Mar 25, 2024

klieret commented Mar 22, 2024

klieret Mar 22, 2024

CedricLeon Mar 25, 2024

CedricLeon commented Mar 25, 2024 •

edited

Loading

CedricLeon commented Mar 25, 2024 •

edited

Loading

klieret commented Mar 25, 2024 •

edited

Loading

CedricLeon commented Mar 26, 2024

CedricLeon commented Mar 28, 2024

klieret commented Apr 1, 2024

klieret commented Apr 1, 2024

CedricLeon commented Apr 4, 2024

CedricLeon commented Apr 18, 2024

Add a frequency parameter to TriggerWandbSyncLightningCallback #101

Are you sure you want to change the base?

Add a frequency parameter to TriggerWandbSyncLightningCallback #101

Conversation

CedricLeon commented Mar 22, 2024

semanticdiff-com bot commented Mar 22, 2024 • edited Loading

klieret Mar 22, 2024

Choose a reason for hiding this comment

CedricLeon Mar 25, 2024

Choose a reason for hiding this comment

klieret commented Mar 22, 2024

klieret Mar 22, 2024

Choose a reason for hiding this comment

CedricLeon Mar 25, 2024

Choose a reason for hiding this comment

CedricLeon commented Mar 25, 2024 • edited Loading

CedricLeon commented Mar 25, 2024 • edited Loading

klieret commented Mar 25, 2024 • edited Loading

CedricLeon commented Mar 26, 2024

CedricLeon commented Mar 28, 2024

klieret commented Apr 1, 2024

klieret commented Apr 1, 2024

CedricLeon commented Apr 4, 2024

CedricLeon commented Apr 18, 2024

Add a `frequency` parameter to `TriggerWandbSyncLightningCallback` #101

Add a `frequency` parameter to `TriggerWandbSyncLightningCallback` #101

semanticdiff-com bot commented Mar 22, 2024 •

edited

Loading

CedricLeon commented Mar 25, 2024 •

edited

Loading

CedricLeon commented Mar 25, 2024 •

edited

Loading

klieret commented Mar 25, 2024 •

edited

Loading