dspy icon indicating copy to clipboard operation
dspy copied to clipboard

ci(dspy): Add tests to CI

Open isaacbmiller opened this issue 5 months ago • 19 comments

  1. Create a new action to run the tests that @thomasahle implemented. Fix #456
  2. Fix the pre-commit checks workflow
  3. Adjusted the pyproject.toml to have pytest as a dependency.

Note: databricks integration does not pass tests. Commented out as that is the only integration to do so. To be added back at a later date

isaacbmiller avatar Feb 26 '24 02:02 isaacbmiller

The databricks integration is very new so it doesn’t have users yet (merged today). Is unmerging it temporarily useful for this

okhat avatar Feb 26 '24 03:02 okhat

Yes unmerging would unblock this

isaacbmiller avatar Feb 26 '24 04:02 isaacbmiller

Also how do you feel about the pre-commit checks? A lot of the code isn't up to style, and it seems very strict. But this could be the start of actually requiring people's code to meet the style guide

isaacbmiller avatar Feb 26 '24 04:02 isaacbmiller

Imo let’s not worry about style checks just yet

okhat avatar Feb 26 '24 05:02 okhat

And 100% feel free to temporarily remove the databricks integration if you like. We’ll add it back soon

okhat avatar Feb 26 '24 05:02 okhat

I’d like to merge this when you think it’s ready! No rush though

okhat avatar Feb 26 '24 05:02 okhat

Leaving databricks commented out but can also delete files

isaacbmiller avatar Feb 26 '24 05:02 isaacbmiller

Oh looks like import pydantic is failing here. Btw so far I've been relying on setup.py so it's very possible the *.toml is not completely correct, someone created it a while back and it may have some mistakes

okhat avatar Feb 26 '24 05:02 okhat

Yeah weird i already manually added pydantic to the .toml file

isaacbmiller avatar Feb 26 '24 05:02 isaacbmiller

Ready to merge whenever @okhat

We do have 3 different build systems currently and should probably get rid of 2 of them, but that can be further discussion

isaacbmiller avatar Feb 26 '24 06:02 isaacbmiller

Wait don't merge setup.py is broken now

isaacbmiller avatar Feb 26 '24 06:02 isaacbmiller

Ok we good now, setup.py build works

isaacbmiller avatar Feb 26 '24 06:02 isaacbmiller

@isaacbmiller , this is great. I did not go through databrick file since it is commented out, but others looks good to me.

insop avatar Feb 26 '24 08:02 insop

@okhat I am locking pydantic dependency to 2.5.0 because there is a test that specifically relies on the version of pydantic. I would rather merge this than rewrite that test, so I will rewrite the test later

isaacbmiller avatar Feb 26 '24 17:02 isaacbmiller

Hmmm github not adding in recent commits

isaacbmiller avatar Feb 26 '24 18:02 isaacbmiller

Ready to merge

isaacbmiller avatar Feb 26 '24 19:02 isaacbmiller

@okhat Can I merge?

isaacbmiller avatar Feb 26 '24 21:02 isaacbmiller

Hi @isaacbmiller , I just patched the databricks.py file in this latest PR to move the OpenAI dependency within a local function. feel free to check it out if that passes the tests now so we can merge that alongside this PR!

arnavsinghvi11 avatar Feb 26 '24 21:02 arnavsinghvi11

Thanks @isaacbmiller ! ready to merge on @okhat 's go :)

arnavsinghvi11 avatar Feb 26 '24 22:02 arnavsinghvi11

Ok running some checks!

okhat avatar Feb 27 '24 05:02 okhat

Unclear why the first test is failing... I tinkered with poetry but it didn't help

okhat avatar Feb 27 '24 05:02 okhat