Skip to content
This repository was archived by the owner on Aug 28, 2025. It is now read-only.

Conversation

@Borda
Copy link
Contributor

@Borda Borda commented Dec 8, 2021

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

working with bash/shell is just pain 😃

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@Borda Borda added enhancement New feature or request ci/cd labels Dec 8, 2021
@Borda Borda force-pushed the refactor/simplify branch 2 times, most recently from d5b48d3 to c2e6155 Compare December 8, 2021 21:56
@Borda Borda force-pushed the refactor/simplify branch from c2e6155 to 4347bb0 Compare December 8, 2021 22:22
@Borda Borda force-pushed the refactor/simplify branch from 8736d5e to bd67860 Compare December 8, 2021 23:53
@Borda Borda marked this pull request as ready for review December 9, 2021 00:59
@Borda Borda force-pushed the refactor/simplify branch from bb6ee00 to 07a3d77 Compare December 9, 2021 01:09
@Borda
Copy link
Contributor Author

Borda commented Dec 9, 2021

@ethanwharris cold ou pls check that the thumb pat is correct after refactor 🐰

Copy link
Member

@justusschock justusschock left a comment

Choose a reason for hiding this comment

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

Can we not include the thumbnail as png here and maybe store it on s3? Once added we never get rid of it from the history and since it's binary every change will be tracked as a new file.

I know that this file is not that large but since we already have a quite large history and every bit counts, I'd rather not add this

@Borda
Copy link
Contributor Author

Borda commented Dec 9, 2021

I know that this file is not that large but since we already have a quite large history and every bit counts, I'd rather not add this

I would not have much concern about thumbnails, but rather all other illustrations...
on the other hand, when someone accidentally deletes it in S3, it is lost... so that we allow images here but precommut preventing adding large files:
https://github.com/PyTorchLightning/lightning-tutorials/blob/5dd7b1c261a3fb6bd5788a9f13f7e95d3ede4987/.pre-commit-config.yaml#L21

@Borda Borda requested a review from justusschock December 9, 2021 09:49
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM !

Copy link
Member

@ethanwharris ethanwharris left a comment

Choose a reason for hiding this comment

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

@Borda Seems to be working, LGTM 😃

@Borda Borda merged commit 796840b into main Dec 9, 2021
@Borda Borda deleted the refactor/simplify branch December 9, 2021 13:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

ci/cd enhancement New feature or request

5 participants