Skip to content
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.

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

Adding support for persistent workers and logger fix #198

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jlotthammer
Copy link

What is the goal of this PR?

This PR adds enhanced configurability and stability to data loading and logging features within pyrelational's DataManager and LightningModelManager classes. These changes aim to improve both the efficiency of data handling and logging management by adding support for persistent workers in data loading and establishing the ability to pass loggers to the pytorch lightning trainer. This update ensures that data loading can better utilize resources over prolonged tasks and provides support for pytorch lightning loggers.

What are the changes implemented in this PR?

Persistent Workers in DataManager:

The DataManager class now includes an option to keep workers persistent across data loading iterations, controlled by a new loader_persistent_workers parameter. By setting loader_persistent_workers: bool = False by default, backwards compatibility is maintained, but it can be enabled to reduce initialization overhead in multi-epoch training, benefiting users handling larger datasets or needing faster data loader setup times.

In the LightningModelManager class, default logging has been configured using PyTorch Lightning's CSVLogger. If no logger is specified in trainer_config, a CSVLogger is automatically set up to log to config["checkpoints_dir"] consistent with pytorch lightning.

Lastly, the abstract model manager now uses a context manager to load JSON data.

@thomasgaudelet
Copy link
Contributor

Thanks for this! Adding @paulmorio for review! We'll review asap 🙂

Copy link
Contributor

@thomasgaudelet thomasgaudelet left a comment

Choose a reason for hiding this comment

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

Thanks for this!

I only would ask to remove the CSVLogger default.

@@ -60,6 +61,10 @@ def init_trainer(self) -> Tuple[Trainer, ModelCheckpoint]:
config = self.trainer_config
config = _add_pyl_trainer_defaults(config)
callbacks: List[Callback] = []

if config["logger"] is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think default should remain with no logger, at the very least there should be an option to not have a logger.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, Thomas. I was trying to match the Pytorch Lightning API which sets as None and uses default behavior of CSVLogger. To have no logger - consistent with the lightning API - you'd set it as logger=False. So we currently support all this functionality and we match the defaults of PyTorch Lightning. If you want to change it however - we can!

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, but wouldn't that mean that we don't need this since by default None would get converted as CSVLogger anyway by the Trainer?

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.

2 participants