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

Introduce RenateLightningModule #301

Merged
merged 35 commits into from
Jun 21, 2023
Merged

Introduce RenateLightningModule #301

merged 35 commits into from
Jun 21, 2023

Conversation

wistuba
Copy link
Contributor

@wistuba wistuba commented Jun 14, 2023

Split Learner into two classes. RenateLightningModule contains the basic logic while Learner adds all CL-related stuff such as val_buffer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@wistuba wistuba changed the base branch from main to dev June 14, 2023 14:30
@github-actions
Copy link

github-actions bot commented Jun 15, 2023

Coverage report

The coverage rate went from 85.68% to 85.21% ⬇️

95.93% of new lines are covered.

Diff Coverage details (click to unfold)

src/renate/data/data_module.py

100% of new lines are covered (93.33% of the complete file).

src/renate/updaters/experimental/repeated_distill.py

100% of new lines are covered (100% of the complete file).

src/renate/updaters/experimental/er.py

100% of new lines are covered (91.61% of the complete file).

src/renate/updaters/avalanche/model_updater.py

100% of new lines are covered (94.57% of the complete file).

src/renate/benchmark/scenarios.py

91.3% of new lines are covered (88.23% of the complete file).
Missing lines: 89, 429

src/renate/updaters/experimental/offline_er.py

100% of new lines are covered (81.08% of the complete file).

src/renate/utils/avalanche.py

83.33% of new lines are covered (98.85% of the complete file).
Missing lines: 27

src/renate/benchmark/experiment_config.py

100% of new lines are covered (96.03% of the complete file).

src/renate/updaters/learner.py

96.61% of new lines are covered (97.28% of the complete file).
Missing lines: 219, 220

src/renate/evaluation/evaluator.py

100% of new lines are covered (96.49% of the complete file).

src/renate/benchmark/experimentation.py

100% of new lines are covered (94.28% of the complete file).

src/renate/updaters/experimental/gdumb.py

100% of new lines are covered (94.44% of the complete file).

src/renate/updaters/model_updater.py

100% of new lines are covered (95.2% of the complete file).

src/renate/updaters/experimental/joint.py

100% of new lines are covered (95.83% of the complete file).

@wistuba wistuba requested a review from prabhuteja12 June 15, 2023 13:38
@@ -317,7 +318,7 @@ def train_transform(dataset_name: str) -> Optional[transforms.Compose]:
raise ValueError(f"Unknown dataset `{dataset_name}`.")


def test_transform(dataset_name: str) -> Optional[transforms.Normalize]:
def test_transform(dataset_name: str) -> Optional[Union[transforms.Normalize, transforms.Compose]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be made a callable? This is very specific to existing transformations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this also Optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it can return None

self._seed = seed
self._task_id: str = defaults.TASK_ID
self._train_dataset = None
Copy link
Contributor

Choose a reason for hiding this comment

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

no typing :)

def training_step_unpack_batch(
self, batch: Tuple[NestedTensors, torch.Tensor]
) -> Tuple[NestedTensors, Any]:
inputs, targets = batch
Copy link
Contributor

@prabhuteja12 prabhuteja12 Jun 16, 2023

Choose a reason for hiding this comment

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

This choice of what a batch comprises is specific to HF vs non-HF datasets and whether there is a buffer? So, why is this specified in the base class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I guess I should change to Any

@@ -253,9 +217,15 @@ def training_epoch_end(self, outputs: List[Union[Tensor, Dict[str, Any]]]) -> No
if not self.val_enabled:
self._log_metrics()

def validation_step_unpack_batch(
self, batch: Tuple[NestedTensors, torch.Tensor]
) -> Tuple[NestedTensors, Any]:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above.

@@ -78,7 +78,7 @@ def execute_job():
mode="max",
config_space=config_space,
metric="val_accuracy",
max_time=30,
max_time=35,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is continually increasing.

Copy link
Contributor

@prabhuteja12 prabhuteja12 left a comment

Choose a reason for hiding this comment

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

Few small changes. I agree with the overall structure of what is being accomplished.

@wistuba wistuba requested a review from prabhuteja12 June 20, 2023 07:02
@wistuba wistuba merged commit c526510 into dev Jun 21, 2023
@wistuba wistuba deleted the mw-simple-learner branch June 21, 2023 13:44
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.

None yet

2 participants