#73 Fix error in check_setup when freezing dependencies

gentzkow / template_archive

20 stars 36 forks source link

#73 Fix error in check_setup when freezing dependencies #74

Closed jc-cisneros closed 1 year ago

jc-cisneros commented 1 year ago

This PR closes #73. The goal of this issue was to address a bug flagged by @zkashner on check_setup.py. Per https://github.com/gentzkow/template/issues/73#issuecomment-1382937861, we currently want check_setup.py to (i) allow fixing packages to a specific version and (ii) checking that all required packages have been installed (independent of their version).

The steps to check the current solution is working as expected are the following:

Clone template and specify a version for any of the packages listed in conda_env.yaml
Create the template conda environment and run check_setup.py. Confirm this script runs without any errors.

@snairdesai let me know if you have the bandwidth to briefly review this PR.

snairdesai commented 1 year ago

Thanks @jc-cisneros! Will get to this PR today.

snairdesai commented 1 year ago

@jc-cisneros Apologies for the delay here, working through this now. Just a quick note that when I rebuild the conda environment with the version number you've specified for termcolor, I get the following error:

I've changed the suffix for a specific version of NumPy just in case this is script related, rather than dependency related.

snairdesai commented 1 year ago

Update:

When I remove the package freeze for TermColor and specify a freeze for NumPy, the environment solves. We want to be sure whatever dependencies we merge back to master here properly solve (and will continue to do so even given underlying changes in the packages themselves).

snairdesai commented 1 year ago

@jc-cisneros Another update here. When I specify a specific version of NumPy, the environment solves as noted above, but check_setup.py fails.

specified conda environment

check_setup.py error message

I'll look into this further, but it seems this is indeed script related. Let me know your thoughts.

snairdesai commented 1 year ago

@jc-cisneros I determined the error - there must be no space when we specify the package dependencies (see screenshot):

Flagging this still doesn't work for the TermColor version specified in the commit above. Once this spacing fix was made, check_setup.py ran as expected:

I propose the following:

Directly specify somewhere in the instructions for conda_env.yaml that there can be no spaces in our suffixes.
Ensure that we merge back to master only an example package which exists and will remain stable within conda.

Once these are resolved, this looks good to merge. Let me know if this makes sense and if you have any thoughts!

jc-cisneros commented 1 year ago

Nice catch @snairdesai! The syntax is then coherent with what you would be running on the command line (i.e., conda install -n template numpy=1.24.0). I agree with your proposal and would add this consideration to the error message you got in https://github.com/gentzkow/template/pull/74#issuecomment-1428399573.