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

Cache LFS objects in the tests workflow #2194

Merged
merged 6 commits into from
Feb 20, 2023

Conversation

atharva-2001
Copy link
Member

📝 Description

Type: 🪲 bugfix | 🚀 feature | ☣️ breaking change | 🚦 testing | 📝 documentation | 🎢 infrastructure

Write a complete description of your changes, including the necessary context or any piece of information required to understand your work.

Also, link issues affected by this pull request by using the keywords: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • My changes can't be tested (explain why)

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently.

Please add your name and email to .mailmap in your current branch and push the changes to this pull request.

In case you need to map an existing alias, follow this example.

@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #2194 (38fe960) into master (5e107ea) will not change coverage.
The diff coverage is n/a.

❗ Current head 38fe960 differs from pull request most recent head 8e68779. Consider uploading reports for the commit 8e68779 to get more accurate results

@@           Coverage Diff           @@
##           master    #2194   +/-   ##
=======================================
  Coverage   71.81%   71.81%           
=======================================
  Files         133      133           
  Lines       12355    12355           
=======================================
  Hits         8873     8873           
  Misses       3482     3482           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sonachitchyan
Copy link
Member

@atharva-2001 be sure to add your info into the .mailmap

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently.

Please add your name and email to .mailmap in your current branch and push the changes to this pull request.

In case you need to map an existing alias, follow this example.

@atharva-2001
Copy link
Member Author

atharva-2001 commented Jan 24, 2023

The tests here are not failing, but the tests here are. I wonder why that is happening. However, the files are being cached!

@atharva-2001 atharva-2001 marked this pull request as ready for review January 24, 2023 12:54
@epassaro
Copy link
Member

epassaro commented Jan 24, 2023

I don't think this is going to work as expected.

You need to use a conditional step, like we do in this example:

      - uses: actions/cache@v2
        with:
          path: ${{ matrix.prefix }}
          key: conda-${{ matrix.label }}-${{ hashFiles('conda-${{ matrix.label }}.lock') }}-${{ env.CACHE_NUMBER }}
        id: cache-conda

      - name: Update environment
        run: mamba update -n tardis --file conda-${{ matrix.label }}.lock
        if: steps.cache-conda.outputs.cache-hit != 'true'

In the example, the second step only runs if the cache exists. In your code, all the steps are running on every commit.

I think this could be achieved by doing:

  1. Run the cache step.
  2. Clone the repo without the LFS objects (there's an option for that)
  3. Get the LFS assets (I'm assuming this step is going to work independently of pulling the LFS objects, but I don't know)
  4. Make a conditional step that pulls the LFS objects if the cache does not exist.

@atharva-2001
Copy link
Member Author

I think this could be achieved by doing:

  1. Run the cache step.
  2. Clone the repo without the LFS objects (there's an option for that)
  3. Get the LFS assets (I'm assuming this step is going to work independently of pulling the LFS objects, but I don't know)
  4. Make a conditional step that pulls the LFS objects if the cache does not exist.

But this is what I'm doing here right? Here is what I think the workflow does:

  1. Clone the repo without LFS Objects
  2. Generate a file(.lfs-assets-id) with all the hashes of the file names. This file is then used to generate the cache key. If any of these files change, then the key would be invalid.
  3. If the cache key matches, the items would be restored and git lfs pull wouldn't do anything. If it fails, there would be nothing in the lfs folder and the items would be downloaded(that way we won't need a conditional statement.)

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently.

Please add your name and email to .mailmap in your current branch and push the changes to this pull request.

In case you need to map an existing alias, follow this example.

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently.

Please add your name and email to .mailmap in your current branch and push the changes to this pull request.

In case you need to map an existing alias, follow this example.

@epassaro
Copy link
Member

I think you are right @atharva-2001, but I think we should keep the lfs: false flag and the conditional statement explicit if the result is the same, just to be more clear in case we need to revisit this in the future.

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently.

Please add your name and email to .mailmap in your current branch and push the changes to this pull request.

In case you need to map an existing alias, follow this example.

@andrewfullard
Copy link
Contributor

I think you are right @atharva-2001, but I think we should keep the lfs: false flag and the conditional statement explicit if the result is the same, just to be more clear in case we need to revisit this in the future.

Why lfs: false? Confusing to me.

@atharva-2001
Copy link
Member Author

Why lfs: false? Confusing to me.

The LFS objects would be saved as cache and we wouldn't need to get them every time the workflow runs. We would only need to get LFS objects if the reference data repository is updated(because that would invalidate the cache key) and we are doing that using git lfs pull.

@atharva-2001 atharva-2001 requested a review from chvogl February 2, 2023 16:24
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
@andrewfullard andrewfullard requested a review from chvogl February 8, 2023 15:02
andrewfullard
andrewfullard previously approved these changes Feb 8, 2023
chvogl
chvogl previously approved these changes Feb 13, 2023
Copy link
Contributor

@chvogl chvogl left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@chvogl chvogl requested review from chvogl February 13, 2023 16:07
@chvogl
Copy link
Contributor

chvogl commented Feb 13, 2023

@atharva-2001 Any idea why the tests are failing?

@atharva-2001
Copy link
Member Author

I'm afraid I'm yet to look at it. I'd soon fix this and let you know!

andrewfullard
andrewfullard previously approved these changes Feb 20, 2023
Copy link
Contributor

@chvogl chvogl left a comment

Choose a reason for hiding this comment

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

Looks great!

@andrewfullard andrewfullard merged commit 2e04916 into tardis-sn:master Feb 20, 2023
epassaro added a commit to epassaro/tardis that referenced this pull request Mar 21, 2023
* Adding rate matrix index (tardis-sn#2132)

* adding NlteIndexHelper class

* adding nlte_ionization_species to io/schemas/plasma.yml

* adding nlte_excitation_species to io/schemas/plasma.yml

* added nlte_ionization_species in assemble_plasma

* fixed the issue with getting nlte_ionization_species from config

* fixed transforming string from config to tuple for nlte ionization species

* ran black

* attempt of writing tests 1

* ran black

* test attempt 2

* black on test_hdf_plasma

* experimenting with revert

* attempt 3

* reverted tardis/plasma/tests/test_complete_plasmas.py

* reverted tardis/plasma/tests/test_hdf_plasma.py

* adding cofig for nlte

* adding fixture for tardis_cofig_verysimple_nlte

* added test_plasma_nlte_section_config

* ran black

* fixed the issue with the test, ran it locally

* ran black on necessary files

* updated description in schemas

* switching from nlte_ionization to be used from self

* ran black

* changed nlte_properties_new to nlte_solver_properties

* adding nlte_rate_equation_matrix.py

* reverting to previous commit

* Restructuring NumbaModel (tardis-sn#2136)

* Added NumbaRadial1DGeometry

* Added to_numba function

* Fixed incorrect naming of numba spec

* Updated docstring

* Updated to_numba docstring

* Updated docstring

* Updated numba model to contain only time explosion

* updated docstring formatting error

* Moved geometry to new geometry sub package

* Updated all functions where numba model was used

* Updated to specify units

* converting time_explosion to seconds specifically

* Added numba geometry fixture

* Updated tests to work with numba geometry

* Updated formatting

* Fixed issue with geometry initialization

* Fixed formatting

* Add missing cell to download atom data (tardis-sn#2146)

* Possible fix for prerelease workflow (tardis-sn#2127)

* Possible fix for prerelease workflow

* Fix double @ symbol

* Changed to v2

* Pin setuptools_scm to v6 (tardis-sn#2147)

* Pin setuptools_scm to v6

* Fix typo

* Adding nlte_rate_equation_solver.py (tardis-sn#2140)

* added nlte_rate_equation_matrix

* adding the rate_equation_matrix

* moved rate_matrix into a class NLTERateEquationSolver

* fixed a bug in rate_matrix_index, added an example for checking if rate matrix works properly

* changed the initial electron density for now

* added tests, fixed a small bug

* added some doctrings

* fixed a typo

* added some doctrings

* added some doctrings part 2

* added some docstrings part 3

* removed unnecessary import

* Update tardis/plasma/properties/nlte_rate_equation_solver.py

Co-authored-by: Christian Vogl <[email protected]>

* changed how atomic number is created from rate_matrix_index

* changed call of a function

* got rid of unnecessary if statement in set_nlte_ion_rate

* renamed last_row to charge_conservation_row

* switched 0, 1 to atomic_number and ion_number to make it more readable

* swtihed from rates to coefficients

* changed the matrix set up to only keep necessary row for nlte_ion

* ran black

* fixing some doctrings

* swtiched from using numbers to index names

* switched the return statement to NotImplementedError

* changed groupby from 0, 1 to atomic number, ion number

* fixed an issue in the test

* ran black

Co-authored-by: Christian Vogl <[email protected]>

* Set specific qgrid feedstock version (tardis-sn#2152)

* Pre-release 2022.11.17 (tardis-sn#2155)

Automated changes for pre-release 2022.11.17

* Post-release 2022.11.17 (tardis-sn#2156)

Automated changes for post-release 2022.11.17

* Pre-release 2022.11.20 (tardis-sn#2160)

Automated changes for pre-release 2022.11.20

* Fix relativistic packet initialization (tardis-sn#2159)

* Fix setting of full relativity flag

* Add relativistic packet source

Co-authored-by: Stuart Sim <[email protected]>

* Fix relativistic vpackets on inner boundary

Co-authored-by: Stuart Sim <[email protected]>

* Fixing typos for nlte ion (tardis-sn#2154)

* fixing typos on rate_matrix

* changed the np arange to use 0 instead of 0.0

* NLTE jacobian matrix (tardis-sn#2158)

* added jacobian matrix and deriv block functions

* added doctrings

* ran black

* switched from df to array

* added an initial test

* ran black

* reorganized the tests

* added doctrings

* added todo comments for future work

* ran black

* Kilonova missing zeta (tardis-sn#2150)

* solved KN ZetaData issue

* KN missing Zeta fixed

* mailmap changed

* Pre-release 2022.12.11 (tardis-sn#2172)

Automated changes for pre-release 2022.12.11

* Post-release 2022.12.12 (tardis-sn#2173)

Automated changes for post-release 2022.12.12

* Add missing __init__.py files to transport and geometry subpackages (tardis-sn#2170)

* Add missing __init__.py files to transport subpackage

* Add missing __init__.py file to geometry subpackage

* Pre-release 2022.12.25 (tardis-sn#2180)

Automated changes for pre-release 2022.12.25

* Post-release 2022.12.26 (tardis-sn#2182)

Automated changes for post-release 2022.12.26

* Pre-release 2023.01.08 (tardis-sn#2186)

Automated changes for pre-release 2023.01.08

* Post-release 2023.01.11 (tardis-sn#2188)

Automated changes for post-release 2023.01.11

* Downloading nlte_atom_data in ref data (tardis-sn#2187)

adding nlte_atom_data to be downloaded

* Adding nlte solver (tardis-sn#2171)

* adding solver part in the calculate method

* adding the objective function

* added the solution vector method

* final touches for the solver

* tests, attempt 1

* ran black

* fixed the issue with tests

* ran black

* added missing doctrings in the nlte_rate_equation_solver.py

* added doctrings to tests

* changed the test to explicitly calculate the lte solution ion number densities.

* ran black

* added the atom data file to download_reference_data.sh

* fixed download_reference_data.sh

* Revert "added the atom data file to download_reference_data.sh"

This reverts commit 58ce29e.

* got rid of index i, kept only shell

* switched DataFrame to pandas.DataFrame in the docstrings

* docstring bug fix

* got rid of the deepcopy of nlte atomic data file

* changed the number of shells to 5

* Update tardis/io/tests/data/tardis_configv1_nlte.yml

Co-authored-by: Christian Vogl <[email protected]>

* Update tardis/plasma/properties/nlte_rate_equation_solver.py

Co-authored-by: Christian Vogl <[email protected]>

* Update tardis/io/tests/data/tardis_configv1_nlte.yml

Co-authored-by: Christian Vogl <[email protected]>

* Update tardis/io/tests/data/tardis_configv1_nlte.yml

Co-authored-by: Christian Vogl <[email protected]>

* ran black

* Update tardis/plasma/properties/nlte_rate_equation_solver.py

Co-authored-by: Christian Vogl <[email protected]>

* Update tardis/plasma/tests/test_nlte_solver.py

Co-authored-by: Christian Vogl <[email protected]>

* restructured a summary in a docstring for the solution_vector_block

* ran black

* added the test for w=0 case

* Update tardis/plasma/properties/nlte_rate_equation_solver.py

Co-authored-by: Christian Vogl <[email protected]>

* Update tardis/plasma/properties/nlte_rate_equation_solver.py

Co-authored-by: Christian Vogl <[email protected]>

* Update tardis/plasma/tests/test_nlte_solver.py

Co-authored-by: Christian Vogl <[email protected]>

* fixed an issue in a docsting

* fixed an issue in a docsting

* removed a TODO comment

Co-authored-by: Christian Vogl <[email protected]>

* Rename T variables to temperature  (tardis-sn#2185)

* fixes tardis-sn#1600

* fixes tardis-sn#1600

* Update tardis/montecarlo/packet_source.py

Co-authored-by: Atharva Arya <[email protected]>

* fixes codestyle

* improve codestyle

* rename the function

* Update packet_source.py

* improve codequality

* add commit

* Update packet_source.py

Co-authored-by: Atharva Arya <[email protected]>

* Fixing test_store_runner_to_hdf (tardis-sn#2198)

* checking if the object has the attribute decode

* fixed another decode issue

* Reading nlte_excitation_species from config (tardis-sn#2195)

* implementing nlte_excitation

* ran black

* fixed a typo

* got rid of unnecessary lines

* rewrote tests

* made a change on assigning the config values to plasma properties

* fixed the tests

* Update tardis/io/tests/test_config_reader.py

Co-authored-by: Christian Vogl <[email protected]>

---------

Co-authored-by: Christian Vogl <[email protected]>

* Fix config reader tests (tardis-sn#2200)

Fix config tests

* Pre-release 2023.02.05 (tardis-sn#2205)

Automated changes for pre-release 2023.02.05

* Post-release 2023.02.06 (tardis-sn#2206)

Automated changes for post-release 2023.02.06

* Add version tag to simulation objects (tardis-sn#2190)

* Add test for versioning info

* Add version string to simulation objects

* Add name to mailmap

* Add another alias to mailmap

* Pre-release 2023.02.12 (tardis-sn#2208)

Automated changes for pre-release 2023.02.12

* Post-release 2023.02.16 (tardis-sn#2210)

Automated changes for post-release 2023.02.16

* Pre-release 2023.02.19 (tardis-sn#2211)

Automated changes for pre-release 2023.02.19

* Cache LFS objects in the tests workflow (tardis-sn#2194)

* Cache LFS objects (actions/checkout#165)

* Delete steps to see file size

Delete commented step which used to download LFS objects using the bash script

* Add lfs:false flag

* Add my username to .mailmap

* Cache hit and allow pytest to run all tests

* Do git lfs checkout if the cache key is found

* Post-release 2023.02.20 (tardis-sn#2213)

Automated changes for post-release 2023.02.20

* Correct the description of 'no_of_packets' in Monte Carlo Configuration (tardis-sn#2214)

* Correct the description of 'no_of_packets' in Monte Carlo Configuration

* Added a simple description

* Pre-release 2023.02.26 (tardis-sn#2222)

Automated changes for pre-release 2023.02.26

* Post-release 2023.02.27 (tardis-sn#2223)

Automated changes for post-release 2023.02.27

* Add docstrings to subpackages (tardis-sn#2204)

* add docstring to subpackage grid

* Add docstrings to subpackages

* Change comments to docstrings

* add docstrings to subpackages

* rename gui/__init__.py

* Reformat using black

* add contributor infos to mailmap file

* add extended summary to docstrings

* Add extended summary to plasma/properties

* Update tardis/io/parsers/__init__.py

Co-authored-by: Sona Chitchyan <[email protected]>

---------

Co-authored-by: kim <[email protected]>
Co-authored-by: Sona Chitchyan <[email protected]>

* Docs Fix: Download Atom Data in rpacket_tracking.ipynb (tardis-sn#2236)

Download atom data from the function

* Fix for automerge (tardis-sn#2242)

Use gh CLI to approve pull requests; use new token

* Fix for release dates (tardis-sn#2243)

* MonteCarlo packet progress bar completes to 100% (tardis-sn#2237)

* Refresh packet progress bar after every iteration

* Udpdated .mailmap

* Reformatted with black

* Fix team reviewers on workflows (tardis-sn#2246)

* Pre-release 2023.03.20 (tardis-sn#2247)

Automated changes for pre-release 2023.03.20

* Post-release 2023.03.20 (tardis-sn#2248)

Automated changes for post-release 2023.03.20

* Create new workflow

* Patch for arepo tests

* Restore deleted parameter

* Black format; Add missing skip reason

* Fix typo in comment

* Add more comments

* Create slash command dispatcher

* Changes to dispatcher

* Black format

---------

Co-authored-by: Sona Chitchyan <[email protected]>
Co-authored-by: Satwik Kambham <[email protected]>
Co-authored-by: Andrew <[email protected]>
Co-authored-by: Christian Vogl <[email protected]>
Co-authored-by: tardis-bot <[email protected]>
Co-authored-by: Stuart Sim <[email protected]>
Co-authored-by: gleck97 <[email protected]>
Co-authored-by: ABHISHEK PATIDAR <[email protected]>
Co-authored-by: Atharva Arya <[email protected]>
Co-authored-by: Kim <[email protected]>
Co-authored-by: Abhishek Patidar <[email protected]>
Co-authored-by: Le Truong <[email protected]>
Co-authored-by: kim <[email protected]>
Co-authored-by: Shreyas Singh <[email protected]>
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.

6 participants