build2023/switch to pyproject

[baggiponte]

Hello there! Finally had time to work on #234. I realised that to fully address the issue, I need to first switch the previous settings to a pyproject.toml-based configuration, then start working on the constraints.

Changes

1. Removed the following:

• requirements.txt
• requirements-dev.txt
• MANIFEST.in
• pytest.ini
• setup.py

And replaced their contents with a single pyproject.toml file, following these guides:

• https://packaging.python.org/en/latest/guides/writing-pyproject-toml/
• https://setuptools.pypa.io/en/stable/userguide/pyproject_config.html

2. Modified the CONTRIBUTING.md file accordingly. Now the contributor just needs to run the following line:

[python -m] pip install ".[dev]"

3. Configured the version property to be read directly from nemoguardrdails.__version__ so you just have to change it in one place.
4. Removed setuptools from the dependency list.
5. Moved watchdog to core dependencies.

How to test

All tests run successfully, yay!

To test whether the new setup works, we should push a release to TestPyPI. I don't know what schema you should use. Might just be a dev release like 0.6.1.dev<whatever> since it's just a throwaway.

Questions

1. Why do you package the following non-python files?

  "**/*.yml",
  "**/*.co",
  "**/*.txt",
  "**/*.json",
  "../examples/**/*",
  "../chat-ui/**/*",

2. Would you like to separate the CLI dependencies and make them optional, e.g. pip install nemoguardrails[cli]? This would allow to remove starlette, watchdog, fastapi, uvicorn and typer out of core dependencies.
3. Why are you using both aiohttp and httpx?

Next steps

1. I could move pylint configs under pyproject.toml if you want.
2. Would you like switching from black + isort to ruff? It's blazingly fast. It can replace some functionalities of pylint too.
3. I would try to relax every constraint once at a time and see whether tests fail or not. If they do, I would just place a <= constraint for that lib. Do you think this might work?
4. Think of a different package manager. The TLDR is: if you don't fell like adding new dependencies in the near future, or don't mind manually editing them in pyproject.toml, we can leave it like this. The long story is in the toggle menu below.

Packaging info

Python PEP 517 and 518 specify the behaviours of packaging frontends and backends. Backends like setuptools are what builds the wheels behind the curtains. Frontends usually provide a unified interface (see here) to work with backends, publish packages, manage virtualenvs and other amenities. Frontends that comply with PEPs will be compatibile with any backend.

To the best of my knowledge, PDM is the most feature-complete frontend that complies with PEPs. But this project does not really need to make a switch if the dependencies won't change much in the near future.

This is why I left setuptools as backend. This means the project can be managed as you used to: just add new dependencies by hand to the pyproject.toml's dependencies or optional-dependencies table(s).

In the future, if using pip + venv + setuptools + twine (to publish the package) is too much, you can always switch to PDM. It will require some small changes to pyproject.toml, but not many.

[drazvan]

Thanks @baggiponte!

Let me first answer your questions:

Why do you package the following non-python files?

The core library comes with some pre-defined prompts, Colang flows and examples that are loaded by default into the server interface, if you don't specify a configs folder. Hence why we need to include the additional files. Also, the chat-ui is the web interface for the server.

Would you like to separate the CLI dependencies and make them optional, e.g. pip install nemoguardrails[cli]? This would allow to remove starlette, watchdog, fastapi, uvicorn and typer out of core dependencies.

Not sure. It might make more sense to have a nemoguardrails[core] which doesn't include the CLI/Server dependencies. If you use only the Python API, then this would be enough. Let me think on this.

Why are you using both aiohttp and httpx?

No strong reason here. We just used a piece of code that used httpx and first included as is. We should refactor at some point.

Next Steps

Regarding the next steps:

I could move pylint configs under pyproject.toml if you want.

Let's leave this for now. We're not using pylint as part of CI just yet.

Would you like switching from black + isort to ruff? It's blazingly fast. It can replace some functionalities of pylint too.

I'll need to look into this. I haven't used ruff.

I would try to relax every constraint once at a time and see whether tests fail or not. If they do, I would just place a <= constraint for that lib. Do you think this might work?

Yes, that should work. We don't yet have 100% code coverage with the tests, so there is still a risk that something might break. But the critical part should be covered.

Think of a different package manager. The TLDR is: if you don't fell like adding new dependencies in the near future, or don't mind manually editing them in pyproject.toml, we can leave it like this. The long story is in the toggle menu below.

We don't expect dependencies to change a lot. If anything, we want to simplify and have as few as possible. So, let's stick with the pyproject.toml file for now. I will test PDM on a different project mean while.

Thanks again for putting the effort to make this conversion!

[drazvan]

Should we merge this and have a separate PR to relax the dependencies?

[baggiponte]

Hey, sorry for the late reply. A bit busy, will get back to this later this week if you don't mind.

[ninjapenguin]

👋 I had actually prepared a similar PR before noticing this had already been done although I think this is better as I'd also switched to hatchling for build as I wasn't as well versed in setuptools so its much nicer to change fewer things!

My motivation for the PR was that we're also unable to use the project in its current state due to the pinned dependency versions so we're currently experimenting with a fork right now.

That all said I'd be a big +1 on the approach of either having a [core] which was a reduced set and removed the server deps, OR moving the server deps out to a [server] package - I guess that just depends on how core to the product you see the server component

[baggiponte]

Hi @drazvan, finally have some time to get back to this. I agree, the dependencies constraints should be a separate PR.

About ruff: if you like, I can add it now, otherwise it can be a separate PR.

I checked the develop branch and I am still on par with that. Let me know if I should rebase against another branch.

Thanks!

[szaher]

@drazvan should we include #250 in this PR? making sentence-transformers optional?

[baggiponte]

Oh definitely, in the PR it might also be appropriate to include whether you want to split dependencies. It's really straightforward, can do that.