Stateful Notebooks

[shubham3121]

Description

Please include a summary of the change, the motivation, and any additional context that will help others understand your PR. If it closes one or more open issues, please tag them as described here.

Affected Dependencies

List any dependencies that are required for this change.

How has this been tested?

• Describe the tests that you ran to verify your changes.
• Provide instructions so we can reproduce.
• List any relevant details for your test configuration.

Checklist

• I have followed the Contribution Guidelines and Code of Conduct
• I have commented my code following the OpenMined Styleguide
• I have labeled this PR with the relevant Type labels
• My changes are covered by tests

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[yashgorana]

are we planning on adding the checkpoint code to all the notebooks in this PR?

[yashgorana]

using dev-latest creates issues with latest image not getting pulled during k8s development.. can we avoid adding this?

[shubham3121]

What are issues we face ? Right now the problem is that we want a way to spin up a cluster with the same tag as checkpoint is created, but since the DEVSPACE_TIMESTAMP is dynamic we cannot create images with a static tag. Also, Currently we're tagging an image with both DEVSPACE_TIMSTAMP tag and tag-latest.

[yashgorana]

The issue is that image:dev-latest does not always get updated if you re-deploy after any code changes, breaking development experience and creating confusion. I previously had to revert this exact change from Rasswanth as well - because it just never worked.

dev-latest might work in CI because there's only one image, but let's not add dev-latest anywhere as it ends up creating confusion, and just causes divergence in expectation while debugging locally, in notebooks and CI.

[shubham3121]

But if the image tag is constantly changing, then the info about the image tag in the db is stale right ? Means the pools cannot use the redeployed versions anyways

[shubham3121]

As per discussion:

1. set the tags such that timestamp is always on top.
   • dev-${DEVSPACE_TIMESTAMP}
   • dev-latest
2. don't change global.version value - keep it dev-${DEVSPACE_TIMESTAMP},
3. use dev-latest for worker pool

Resolved in: 9e98cf4

[shubham3121]

are we planning on adding the checkpoint code to all the notebooks in this PR?

Yes planning to do that as a new PR. We will create checkpoint at end of each notebook and then upload them as artifacts in CI.

[yashgorana]

are we planning on adding the checkpoint code to all the notebooks in this PR?

Yes planning to do that as a new PR. We will create checkpoint at end of each notebook and then upload them as artifacts in CI.

Let's not use artifacts; reason being 1) issues with conflicting caches across multiple version/branches/nightlies/pr like we have with current pip cache 2) goals was so that local testing can leverage these checkpoint files directly.

I'd recommend keeping it simple and checking them into the repo itself. That way everyone can run it locally almost instantly and we don't have to worry about wrong checkpoint versions being loaded for some other syft version on CIs.

[shubham3121]

are we planning on adding the checkpoint code to all the notebooks in this PR?

Yes planning to do that as a new PR. We will create checkpoint at end of each notebook and then upload them as artifacts in CI.

Let's not use artifacts; reason being 1) issues with conflicting caches across multiple version/branches/nightlies/pr like we have with current pip cache 2) goals was so that local testing can leverage these checkpoint files directly.

I'd recommend keeping it simple and checking them into the repo itself. That way everyone can run it locally almost instantly and we don't have to worry about wrong checkpoint versions being loaded for some other syft version on CIs.

But don't we do that for K8s logs ? For each k8s run in CI the logs are available for us to download at end of tests. We were thinking of a similar behaviour for checkpoints as well, atleast when tests fail.

I also agree local testing is the priority but I still think we can leverage it in CI atleast for notebooks and maybe just scenario notebooks.

Also caching uses a different plugin and we don't want to persist checkpoints across PRs, instead have a copy available if things break in CI for local debugging.

[yashgorana]

are we planning on adding the checkpoint code to all the notebooks in this PR?

Yes planning to do that as a new PR. We will create checkpoint at end of each notebook and then upload them as artifacts in CI.

Let's not use artifacts; reason being 1) issues with conflicting caches across multiple version/branches/nightlies/pr like we have with current pip cache 2) goals was so that local testing can leverage these checkpoint files directly.
I'd recommend keeping it simple and checking them into the repo itself. That way everyone can run it locally almost instantly and we don't have to worry about wrong checkpoint versions being loaded for some other syft version on CIs.

But don't we do that for K8s logs ? For each k8s run in CI the logs are available for us to download at end of tests. We were thinking of a similar behaviour for checkpoints as well, atleast when tests fail.

I also agree local testing is the priority but I still think we can leverage it in CI atleast for notebooks and maybe just scenario notebooks.

Also caching uses a different plugin and we don't want to persist checkpoints across PRs, instead have a copy available if things break in CI for local debugging.

ah i must've confused cache with artifacts. okay this looks cool.

[yashgorana]

awesome work @shubham3121 @rasswanth-s =)