BasisResearch / cities

Home of Basis development for the 2023 TOP Sprint
MIT License
6 stars 0 forks source link

add CausalInsightSlim class to completely eliminate reliance on torch in the slim version #100

Closed rfl-urbaniak closed 6 months ago

rfl-urbaniak commented 6 months ago

pip install -e .

Do not try install stuff from requirements.txt!! This file has been removed as it's been superseded by layered requirements that now live in setup.py.

After the installation double check Torch, Pyro and ChiRho are absent from your environment.

Then the only change needed is replacement of imports of CausalInsight with

from cities.queries.causal_insight_slim import CausalInsightSlim as CausalInsight, as illustrated in the most recent version of slider.ipynb.

emackev commented 6 months ago

@rfl-urbaniak , I tried installing without requirements.txt, and a fresh venv, and pip install -e .

I'm running into a couple issues:

I'm on staging-county-data branch. Any insights?

Ideally before the TOP demo, the install instructions on the git readme should run without errors. image

emackev commented 6 months ago

@rfl-urbaniak , I tried installing without requirements.txt, and a fresh venv, and pip install -e .

I'm running into a couple issues:

  • pytest isn't in setup.py
  • when I install from dev branch (reminder, mac computers need quotes like this: pip install -e .'[dev]'), I can run pytests, but some of the tests don't pass (see attached)

I'm on staging-county-data branch. Any insights?

Ideally before the TOP demo, the install instructions on the git readme should run without errors. image

We can remove the line about requirements.txt, and the line about running pytests.

rfl-urbaniak commented 6 months ago

Yeh this is exactly what I said. Inference tests fail with new data unless we retrain the models. Moreover the slim version which is the default has only the dependencies needed for frontend. You won't be able to run training and many other things in the default minimal version. To be able to run everything you need the dev version which it seems you correctly installed. The failure of test_inference is just what I said would happen. For now if it's at the staging brain we can live with that. Once we gather a bunch of variables I'll retrain the models and this should go away.

rfl-urbaniak commented 6 months ago

Pytest is in setup.py but not in the minimal install. The stratification of requirements is completely intended, chat with Ria I explained this to her at some point

rfl-urbaniak commented 6 months ago

What branch are you at?

emackev commented 6 months ago

Gotcha, fine with tests failing for now on the staging branch, as we discussed earlier. I'm on staging-county-data.

On the main github readme, it still says to install from requirements.txt, and run pytests -- is this intended?

https://github.com/BasisResearch/cities/tree/main

python -m venv venv
source venv/bin/activate
pip install -r requirements.txt
pip install -e .
cd tests && python -m pytest
rfl-urbaniak commented 6 months ago

Nah, requirements.txt is dead. The instructions are up for revision. Will be cleaned up once this staging lands.

On Fri, Jan 12, 2024, 18:23 emackev @.***> wrote:

Gotcha, fine with tests failing for now on the staging branch, as we discussed earlier. I'm on staging-county-data.

On the main github readme, it still says to install from requirements.txt, and run pytests -- is this intended?

https://github.com/BasisResearch/cities/tree/main

python -m venv venv source venv/bin/activate pip install -r requirements.txt pip install -e . cd tests && python -m pytest

— Reply to this email directly, view it on GitHub https://github.com/BasisResearch/cities/pull/100#issuecomment-1890137572, or unsubscribe https://github.com/notifications/unsubscribe-auth/APEMCZIT6KLJZ7GJJB56OB3YOHAXPAVCNFSM6AAAAABBSMR2TCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJQGEZTONJXGI . You are receiving this because you were mentioned.Message ID: @.***>

emackev commented 6 months ago

Great thanks!

rfl-urbaniak commented 6 months ago

I mean, pls add this to issues on GitHub.

On Fri, Jan 12, 2024, 18:28 Rafal Urbaniak @.***> wrote:

Nah, requirements.txt is dead. The instructions are up for revision. Will be cleaned up once this staging lands.

On Fri, Jan 12, 2024, 18:23 emackev @.***> wrote:

Gotcha, fine with tests failing for now on the staging branch, as we discussed earlier. I'm on staging-county-data.

On the main github readme, it still says to install from requirements.txt, and run pytests -- is this intended?

https://github.com/BasisResearch/cities/tree/main

python -m venv venv source venv/bin/activate pip install -r requirements.txt pip install -e . cd tests && python -m pytest

— Reply to this email directly, view it on GitHub https://github.com/BasisResearch/cities/pull/100#issuecomment-1890137572, or unsubscribe https://github.com/notifications/unsubscribe-auth/APEMCZIT6KLJZ7GJJB56OB3YOHAXPAVCNFSM6AAAAABBSMR2TCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJQGEZTONJXGI . You are receiving this because you were mentioned.Message ID: @.***>