Tox integration, library upgrade, Python 3.11 support #46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Strangely, the reverse lookup for http://www.w3.org/2004/02/skos/core# does not work. It returns only dc. This can be reproduced via the browser as well, which redirects to a page of alternative results instead of SKOS. Try: http://prefix.cc/reverse?uri=http://xmlns.com/foaf/0.1/&format=json A JSON result is returned as expected with expected items. Then try: http://prefix.cc/reverse?uri=http://www.w3.org/2004/02/skos/core%23&format=json A JSON result is returned as expected, but it contains only the (unexpected) dc prefix and corresponding namespace. Go on to http://prefix.cc/ and enter http://www.w3.org/2004/02/skos/core# in the search bar. The redirected page shows dc as the canonical and lists skos as the last alternative (as of date). Somebody should report this but for now, fall back to using the DC pair that's being returned, as the test.
All tests pass for Python 3.8, against present list of requirements, as-is. However, for some reason we do not yet know, Tox cannot run `setup.py` or the setup can't locate the requirements files in the Tox environment. We therefore use `skipsdist=true` for now, leaving the packaging untested. See gitpython-developers/GitPython#1090
Not sure when was the last time https://httpstat.us/ worked, but it doesn't seem to be up anymore or it is unstable. Use https://httpbin.org/ instead.
Python 3.10 onwards dropped support for importing `Mapping` from `collections` directly. Instead, one needs to import from `collections.abc`. This has been supported since Python 3.3, so is backward-compatible with the last LTS Python 3.8.
Pandas 2.0.3 is the last version that supports Python version range 3.8, 3.11. Starting from 1.5.3, specifying `value` is not supported for `df.replace()` when using a nested _dict_ as the `to_replace` value. In such cases, missing value fillers (e.g. using `np.nan`) have to be included in the dictionary itself. Also start ignoring some test output artifacts.
- Run GitHub CI for only used/expected branches in project - Add/modify some comments in Github CI config for clarity - Add GitHub CI matrix strategy for multiple Python environments - Update Actions versions and stick to specific versions - Add a make install target for avoiding redundant pip installs - Use a recent Python version for publishing workflow Note that there may be some redundancy between GitHub's matrix strategy support for multiple environments and Tox. Combining both usually accommodates sophisticated configurations (like multiple Django sub-environments for a particular Python environment), which we don't yet have a need for.
Lock to the latest packages of the lowest-supported Python (3.8). Explicit dependencies only, so as to not confuse people (which is what `pip freeze` is often good at doing). Use a trick like below to manually/ChatGPT inspect/replace existing requirements: `grep -wiFf $requirements_file <(pip freeze)` Remove version specifiers in the requirement files and upgrade dependencies first, then compare the two lists. Pip dependencies like `setuptools` and `wheel` can be and are omitted. We might want to move to listing only parent packages, or to Poetry :)
Attain true cross-platform compatibility by making Github Actions CI run for all three major operating systems, for all runtimes. - Add multi-dimensional environments in CI config - Add job concurrency group (or risk being killed) **Caveat**: Tox appears to run twice on Windows, and sometimes on Mac. This may be because of the system-wide availability of multiple interpreters in some runner types. We either need to decouple Tox from the build (take it out of Makefile) and use the Tox GH Actions plug-in, or add some condition to the job to run Tox for only one runtime/interpreter version.
While `setuptools` and `wheel` should be installed by default in recent Python environments, make no assumption as there are various GNU/Linux distributions, for example, that may not have the same packaging practices.
Python 3.7 and earlier reached EOL earlier this year, and many tools/libraries have stopped supporting those runtime. Continuing to support an old runtime will be near impossible due to dependency hell. It is therefore only wise to follow suit. - Update code, tests - Update CI config - Update dependencies - Update metadata We will aim to support and test against the oldest `security` release and the latest `bugfix` release, less one (if it's too new and not a lot of tools have adopted it). The jump from Python 3.6 to 3.8 doesn't involve that many changes among dependency APIs for this project -- there is just a `collections.Mapping` namespace change and a Pandas `DataFrame.replace()` field behaviour change. Users are encouraged to upgrade even if there is no API or ABI change in this project itself, as an EOL runtime is a security risk.
costezki
approved these changes
Oct 23, 2023
| @@ -16,7 +16,7 @@ Feature: SPARQL query fetcher | |||
| Examples: | |||
| | endpoint_reference | query_text_reference | error_fragment | | |||
| | https://dc51efef6cca43debdb478b330dc7379.com | select * where {?s ?p ?o} limit 10 | urlopen error | | |||
| | https://httpstat.us/500 | select * where {?s ?p ?o} limit 10 | EndPointInternalError | | |||
| | https://httpbin.org/status/500 | select * where {?s ?p ?o} limit 10 | EndPointInternalError | | |||
There was a problem hiding this comment.
Good that you noticed -- did you have a reason to switch to httpstat.us?
| @@ -0,0 +1,60 @@ | |||
| [tox] | |||
| skipsdist = true | |||
| envlist = py38,py311 | |||
There was a problem hiding this comment.
Shall we consider any version in between? Which I guess would not matter much.
There was a problem hiding this comment.
We can. I chose not to because it would add extra build/test time, especially if we test for multiple OSs. But if you think it's the right thing to do, we can list them for sure. Please let me know your preference. If you have none, we will stick with oldest and newest supported.
CaptainOfHacks
previously requested changes
Oct 23, 2023
Move from GPL to Apache
schivmeister
force-pushed
the
feature/MWB-194-tox-ci
branch
from
d16f03c to
82d1ee9
Compare
While removing `value` kept the `to_replace=<dict>` and `regex=True` undocumented approach working -- and we didn't want to "fix what isn't broken" -- it appears more proper to use `regex=<dict>` with `to_replace=None`, which aligns with latest upstream documentation. All tests pass with this change, but no further (manual) inspections have been performed to ensure correctness of the method beyond what is captured by the tests.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #46 +/- ##
==========================================
- Coverage 85.17% 83.97% -1.21%
==========================================
Files 16 14 -2
Lines 533 493 -40
==========================================
- Hits 454 414 -40
Misses 79 79
☔ View full report in Codecov by Sentry. |
We want all jobs in the matrix to continue running even if one or more fails in the same workflow.
schivmeister
force-pushed
the
feature/MWB-194-tox-ci
branch
from
3b8f4f6 to
78ab4d6
Compare
We want to ignore negative coverage reports until we decide to test `__init__.py` files as well in some way.
schivmeister
force-pushed
the
feature/MWB-194-tox-ci
branch
from
039bfd6 to
dd01865
Compare
On Github CI Windows builds appear to be volatile, sometimes erroring out on `pip install`s that are run without the `python -m` prefixed. Since this is the proper way to run `pip` anyway, let's incorporate it into our Makefile, whether or not the error is always reproducible.
One of our tests, which triggers a remote `URITooLong` error, sometimes ends up causing a system/network `BrokenPipeError` instead on the Github Actions CI/CD runners, and possibly in other systems.
|
@costezki @CaptainOfHacks requested changes are in but some of my comments remain unanswered, so did not resolve those. i noticed some more volatility with the CI builds and got stuck fixing things again, but should be good now (i decided to proceed with the workarounds). I tried to get coverage to pass but no dice, so may remove that change (to ignore |
|
Let's proceed with the merge. The coverage difference is insignificant in this case. |
costezki
approved these changes
Oct 24, 2023
costezki
approved these changes
Oct 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reproducing latest commit message as a summary: