Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve developer setup process #25

Merged
merged 23 commits into from
Sep 9, 2022
Merged

Improve developer setup process #25

merged 23 commits into from
Sep 9, 2022

Conversation

riedgar-ms
Copy link
Contributor

@riedgar-ms riedgar-ms commented Aug 5, 2022

Move the developer setup into a dev extra within setup.py.

Also test on Python 3.9, since 3.7 is now security-fixes-only.

@riedgar-ms
Copy link
Contributor Author

Ping..... thoughts @Harsha-Nori @slundberg @Nking92 ?

@slundberg
Copy link
Contributor

Thanks @riedgar-ms! I think the only reason we have requirements.txt is because of some Azure pipeline testing setup limitation. In practice I think the dependencies should live in setup.py with dev dependencies in extras_require. But I would like to hear your thoughts.

https://pip.pypa.io/en/stable/user_guide/?highlight=requirements.txt#requirements-files

@riedgar-ms
Copy link
Contributor Author

@slundberg I'm not an expert in Python packing, but isn't extras_install supposed to be extra dependencies for particular 'bracket installations' of the package. It looks like your recent changes to generators.py mean you're envisaging users doing something like

pip install adatest[openai]

or

pip install adatest[huggingface]

to select different generator back ends?

So in that case, we would be expecting users to do something like:

pip install -e '.[development]'

if they wanted to set up their environment for doing AdaTest development?

@riedgar-ms
Copy link
Contributor Author

Although doing some more reading, it appears that there is a tests_require= section in setup.py these days. Perhaps we could try that instead? WDYT @slundberg ?

@riedgar-ms
Copy link
Contributor Author

@riedgar-ms
Copy link
Contributor Author

I've done a little research on Python packaging, and the options seem to be many, varied and ever changing. One suggestion I did see was that setup.py (or setup.cfg .... one of the various variants) should have the source dependencies, and requirements.txt should have the dev dependencies. So the thing to do would be to remove the existing requirements.txt and rename the requirements-dev.txt in its place.

WDYT @slundberg ?

@slundberg
Copy link
Contributor

slundberg commented Aug 11, 2022

Sorry for the slow reply (sick family this week). I think the purpose of the requirements.txt is to save a snapshot of a system, almost like an "image". We could use it for testing, but then I think that would be more confusing than just an explicit "tests" in extra_require (using it for testing is not one of the 4 options listed in the pip docs). As for bracket installs, for optional dependencies I would expect users to already have the backends they plan to use installed, or they would install them separately. So for example if someone wants to use OpenAI they should install openai, or if they want to use ai21 they should install that, or if they want transformers, or azure, or etc. I don't think all those need to be bracket installs necessarily, and they should not all get installed by default. Thoughts?

@riedgar-ms riedgar-ms changed the title Separate developer requirements Improve developer setup process Aug 15, 2022
@riedgar-ms
Copy link
Contributor Author

@slundberg , I've changed it so that everything is in a [dev] extra... this would actually render the requirements.txt obsolete.

@Harsha-Nori
Copy link
Contributor

Sorry for the slow reply (sick family this week). I think the purpose of the requirements.txt is to save a snapshot of a system, almost like an "image". We could use it for testing, but then I think that would be more confusing than just an explicit "tests" in extra_require (using it for testing is not one of the 4 options listed in the pip docs). As for bracket installs, for optional dependencies I would expect users to already have the backends they plan to use installed, or they would install them separately. So for example if someone wants to use OpenAI they should install openai, or if they want to use ai21 they should install that, or if they want transformers, or azure, or etc. I don't think all those need to be bracket installs necessarily, and they should not all get installed by default. Thoughts?

I like this in general, but also think it doesn't hurt for us to provide bracket installation options for e.g. transformers/openai. The main advantage here is that we can eventually specify min/max version numbers of libraries without taking official dependencies on them. I think this PR sets us up well for that though with the new move to the [dev] tag, so we can add on new extra tags in a subsequent commit.

@riedgar-ms
Copy link
Contributor Author

Ping @slundberg .... do you prefer this approach?

@riedgar-ms riedgar-ms merged commit fa954b3 into microsoft:main Sep 9, 2022
@riedgar-ms riedgar-ms deleted the riedgar-ms/separate-dev-requirements branch September 9, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants