Skip to content

Conversation

ydcjeff
Copy link
Contributor

@ydcjeff ydcjeff commented Nov 11, 2020

Fixes #1403

Description: use actions/cache@v2 for caching

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Nov 11, 2020

@ydcjeff thanks for the PR !

If we would like to use cache, it is better to use it for all github actions:

  • unit-tests.yml
  • hvd-tests.yml
  • tpu-tests.yml
  • ...

Let's start with unit-tests.yml. I'd prefer to have a single step with caching conda and pip folders and cache key depending on the year and week number: date +"%Y-%U" such that we could rebuild cache the next week. What do you think ?

Btw, I'm wondering what is the tool you use for formatting yml, e.g. replacing quotes, indents etc ? I have no strong opinion on formatting those things. However, it is better to keep a single format for all. Let's keep the format as it is for this PR and maybe we can change it later in another PR.

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Nov 11, 2020

Ahhh yes caching with date is a good idea, will play around it. ouch i forgot to add in some github actions.
I used VSCode and it does the formatting (typically https://prettier.io)

EDIT: we can do yaml format with pre-commit

Copy link
Collaborator

@vfdev-5 vfdev-5 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 the update @ydcjeff ! I inspected a bit the logs and looks like there is an issue with pip cache dir fetching.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Nov 11, 2020

@ydcjeff I wonder also how can we check if it works actually : can use the cache ?
Maybe, we can merge that to a branch and create a PR from the branch to the master and see ?

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Nov 11, 2020

@ydcjeff I wonder also how can we check if it works actually : can use the cache ?
Maybe, we can merge that to a branch and create a PR from the branch to the master and see ?

To see if it works is pip install is using cached instead of downloading...

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Nov 11, 2020

@ydcjeff yes, looks like actions/cache downloaded and reused cache (https://github.com/pytorch/ignite/pull/1455/checks#step:6:19), but "Install dependencies" step looks wrong: https://github.com/pytorch/ignite/pull/1455/checks#step:7:24. I'd expect that we do not need to reinstall torch, vision and other deps. Could you please take a deeper look? Thanks

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Nov 12, 2020

@vfdev-5 I see but how shall we handle if there is a new requirement for testing?
We can currently skip the install deps steps if cache hit

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Nov 12, 2020

@ydcjeff we can dump conda list | grep torch and requirements-dev.txt into a file and create a hash of that.

We can currently skip the install deps steps if cache hit

Sounds good

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Nov 12, 2020

@vfdev-5 seems like at least we need to install deps from cache or do you know any other way?
https://github.com/pytorch/ignite/pull/1455/checks?check_run_id=1391042462

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Nov 12, 2020

@ydcjeff thanks a lot for working on that ! Let me check that in more details and we can decide on what can be done.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Nov 23, 2020

@ydcjeff there are several approaches to use cache with setup-miniconda actions. In all the cases we have to run setup-miniconda otherwise we have to manually setup env in order to have conda properly found.

  1. we cache conda's cache and call all steps and it is a tradeoff between downloading the cache vs pip download
  2. either we cache whole folder /usr/share/miniconda3/envs/test and skip "install deps" step, but there an additional conf is needed or maybe conda wont like existing test folder... @goanpeca do you remember what was the issue here ?

@ydcjeff maybe your approach (1) is what we can take here. However, I prefer to follow this guide : https://github.com/conda-incubator/setup-miniconda#caching : cache action is done before setup-miniconda action. Probably, we can define the paths to cache like :

            ~/conda_pkgs_dir
            ~/.cache/pip

for all OS.
To check & debug etc, I'd suggest to create a playground like here and perform several builds to see the caching in action.

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Nov 23, 2020

@vfdev-5 Thank you for your comment. I will checkout from this branch and play around in my forked repo.

@ydcjeff ydcjeff requested a review from vfdev-5 November 24, 2020 16:35
@goanpeca
Copy link
Collaborator

maybe conda wont like existing test folder.

This one :) @vfdev-5

Copy link
Collaborator

@vfdev-5 vfdev-5 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 the updates @ydcjeff

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @ydcjeff for the PR and appologies for the time it took to merge it !

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 3, 2020

@ydcjeff could you please fix the conflicts due to the new yml style

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Dec 3, 2020

@ydcjeff could you please fix the conflicts due to the new yml style

I'm on it.

Copy link
Collaborator

@vfdev-5 vfdev-5 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 the update !
Actually, I checked how cache is used for windows and seems like there is something missing:

@ydcjeff ydcjeff requested a review from vfdev-5 December 3, 2020 15:07
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 3, 2020

@ydcjeff thanks !

@vfdev-5 vfdev-5 merged commit 71d97da into pytorch:master Dec 3, 2020
@ydcjeff ydcjeff deleted the cache branch December 3, 2020 16:46
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.

Use @actions/cache in GH actions
3 participants