Rename _test method in generator.py
#421
Conversation
meta-cla
bot
added
the
CLA Signed
src/forge/actors/generator.py
Outdated
| logger.info("[Generator] save model parameters for testing.") | ||
| await self.generator_worker._test_save_model_params.call() | ||
| async def save_model_params(self): | ||
| """Save model parameters before weight update, used for debugging purposes.""" |
There was a problem hiding this comment.
Nit: Can we bump the for debugging purpose to the front? It's easy to miss at the end of a line
| """Save model parameters before weight update, used for debugging purposes.""" | |
| """(For Debugging/Testing): Save model parameters before weight update.""" |
JenniferWang
force-pushed
the
jiyue/rename-test-policy-update
branch
from
2cbf7b1 to
37e08bf
Compare
src/forge/actors/generator.py
Outdated
| await self.worker._test_save_model_params.call() | ||
| async def save_model_params(self): | ||
| """Used for debugging purpose. Save model parameters before weight update.""" | ||
| logger.info("[Generator] save model parameters for debugging.") |
src/forge/actors/generator.py
Outdated
| return await self.worker._test_validate_model_params.call(validate_fn) | ||
| async def validate_model_params(self, validate_fn): | ||
| """Used for debugging purpose. Validate saved params using validate_fn.""" | ||
| logger.info("[Generator] start validating saved model parameters.") |
There was a problem hiding this comment.
Why remove logging? These methods are only used in tests and won't spam the logs otherwise.
| async def _test_validate_model_params(self, validate_fn): | ||
| """Validate updated model params using validate_fn.""" | ||
| logger.info("[GeneratorWorker] start validating model parameters.") | ||
| async def validate_model_params(self, validate_fn): |
There was a problem hiding this comment.
Actually - could we expose an endpoint to get the underlying model? Then these methods could be entirely deleted, no?
Sorry if I'm misunderstanding. But I don't like having a debug param saved on this model :/
There was a problem hiding this comment.
I think it's a fair tradeoff to expose certain method for debugging purpose.
The beauty of exposing a validator function here is it's a very flexible and reliable way to poke at the property on each worker shard and saves the cost of message SerDer.
Another restriction here is Policy is behind Service interface....
There was a problem hiding this comment.
saves the cost of message SerDer.
Do we imagine testing w/ these methods at a substantial scale?
There was a problem hiding this comment.
saves the cost of message SerDer.
Do we imagine testing w/ these methods at a substantial scale?
My hunch is so long as we're still iterating on weight sync strategy, having some capability to poke into the states across multiple workers under multi-node, multi-replica run might be super valuable.
| async def _test_save_model_params(self): | ||
| """Save model parameters before weight update, used for tesing purposes only.""" | ||
| logger.info("[GeneratorWorker] save model parameters for testing.") | ||
| async def save_model_params(self): |
There was a problem hiding this comment.
Could this method return all the model params? Instead of saving them to the model?
There was a problem hiding this comment.
Sorry I am late to the party. The short answer is no: the workers are remote and it's not feasible to send all the parameters through RPC.
JenniferWang
force-pushed
the
jiyue/rename-test-policy-update
branch
from
37e08bf to
39695a4
Compare
4 participants
Trying to make the
GeneratorAPI a bit cleaner.