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

Add CI requirements update script #2422

Merged
merged 3 commits into from
Nov 13, 2023
Merged

Add CI requirements update script #2422

merged 3 commits into from
Nov 13, 2023

Conversation

texodus
Copy link
Member

@texodus texodus commented Nov 9, 2023

No description provided.

@texodus texodus added the internal Internal refactoring and code quality improvement label Nov 9, 2023
}

fs.writeFileSync(
`python/perspective/requirements-${version.replace(".", "")}.txt`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use this opportunity to move the requirements to a folder and exclude the folder from distribution (in MANIFEST.in)

Copy link
Member Author

@texodus texodus Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

[edit] Even if they were disted - why is that a bad thing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're only used for testing, and it's confusing because many other libraries use similarly named files as their runtime deps

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between CI deps and runtime deps?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When an end user does pip install perspective-python and installs perspective from pypi, they will pull down a dynamic set of dependencies based on their currently installed dependencies and perspective's defined bounds. These are our runtime deps. In CI, we don't do this, we install a fixed set of known-good dependencies to run lints and tests against. These are our CI deps.

Our runtime deps are defined in setup.py, but some repos use requirements.txt. I think it would be good to clearly put these deps in their own folder so people don't look and think "Oh im on Python3.8, perspective only supports the fixed set of pinned dependencies in requirements-38.txt and that doesn't work for me". A good example of this is airflow, which creates the same type of "known-good" requirements files as we do, but they put them in their own space: https://github.com/apache/airflow/tree/constraints-2-7.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd have to dig 2 levels deeps into our source repo to even know these files exist. How is moving them to 3 levels deep going to correct developers' expectations?

Copy link
Member

@timkpaine timkpaine Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're at the root of our python project. Its a tiny change and it doesn't seem like its a big deal from your perspective, so I think its a good thing to do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pandas has the same configuration we have - developer/ci requirements in requirements-dev.txt in the root of the project next to setup.py, and no one is confused. Moreover, the pip documentation says that the file name can be anything, and mentions absolutely nothing about conventional use.

I will change this because I'm tired of discussing it, but I'm doing so under protest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a user who is heavily involved in python packaging, I can confirm that there is a strong convention for storing runtime dependencies in a file named requirements.txt.

Using a file named requirements.txt for anything other than runtime dependencies would be quite surprising/confusing.

The convention is also extended to optional dependencies (or extras) where requirements-<name>.txt maps to the optional dependency group <name>. It is quite often that you will see requirments-dev.txt, requirements-test.txt and requirements-docs.txt files alongside the requirements.txt file.

This convention is strong enough that there is a hatch-requirements-txt plugin for the hatch packaging tool to dynamically map the contents of these files into the pyproject.toml.

requirements-38.txt isn't requirements.txt but perhaps it's close enough to still cause confusion. If it costs nothing to reduce the possibility of confusion, that's probably a good thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what pandas requirements-dev.txt at their project root is used for, but in CI they use sets of requirements from a ci/deps folder. Their per-python version dependencies specified with conda environment.yml files, which is similar to what we're doing with requirements.txt.

https://github.com/pandas-dev/pandas/tree/main/ci/deps

I think it's nice when CI-specific stuff is kept in a separate, clearly labeled place like that. Makes it easy to find them when you need them, and also easy to ignore when you're just doing local development.

@texodus texodus force-pushed the upgrade-emsdk branch 5 times, most recently from 875f9fb to e47e294 Compare November 11, 2023 04:41
@texodus texodus changed the title Upgrade emsdk and add CI requirements update script Add CI requirements update script Nov 11, 2023
@texodus texodus merged commit 401bfb5 into master Nov 13, 2023
12 checks passed
@texodus texodus deleted the upgrade-emsdk branch November 13, 2023 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Internal refactoring and code quality improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants