Transport-for-the-North / Land-Use

NorMITs
GNU General Public License v3.0
0 stars 2 forks source link

pandas / caf.toolkit conflict in environment.yml #42

Open asongtoruin opened 9 months ago

asongtoruin commented 9 months ago

Hi again.

We've possibly found another issue with the environment file as we've been trying to reproduce outputs. The file specifies a dependency on caf.toolkit:

https://github.com/Transport-for-the-North/Land-Use/blob/3ff21685837ee7e4579e424a06a4f37964af6a92/environment.yml#L6

Which in turn has a dependency on pandas>1.4.0 (see here). This version of pandas deprecated DataFrame.append (see here), which is used in various places in this repo, e.g.:

https://github.com/Transport-for-the-North/Land-Use/blob/3ff21685837ee7e4579e424a06a4f37964af6a92/land_use/base_land_use/base_year_population_process.py#L690-L692

There are a few different options, as I see it:

  1. We update these to properly use pd.concat. This would futureproof the code, but also this issue could be widespread (this is just the first time it stopped our process from running)
  2. We reduce / remove the caf.toolkit version requirement - I don't have enough familiarity with this module to know if this would be simple or not. @BenTaylor-TfN any thoughts?
  3. We get a "hard-pinned" environment file from the last time this was run from Someone.
MatteoGravelluTfN commented 9 months ago

Hi Adam, thanks for this. Some comments from me.

  1. I believe this should be a good long-term solution - I know we didn't plan to have considerable changes in the code but this could benefit a robust environment to functions relationship that we might be using from now onwards.
  2. Only comment from me is: I don't specifically know at what point caf.toolkit is used in land use, might be worth a check. I'll let @BenTaylor-TfN comment here.
  3. @chrisbstorey do you remember if we run the code from an internal TfN VM? If not, do you remember who from WSP (?) might have run the code so I can double check with them? Thanks
MatteoGravelluTfN commented 9 months ago

@asongtoruin, I opened the VM that was allegedly used in the past for Land Use and found the following environments:

I will send separately the connection file to the VM and the password, I am sure you can use it as I don't think any other consultancy is using it atm.