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

pre-commit generating incorrect csv example files #285

Closed
odscjen opened this issue May 28, 2024 · 3 comments
Closed

pre-commit generating incorrect csv example files #285

odscjen opened this issue May 28, 2024 · 3 comments
Assignees

Comments

@odscjen
Copy link
Collaborator

odscjen commented May 28, 2024

See 8d6bf03

.manage.py pre-commit was run and it changed the files examples/csv/nodes.csv and examples/csv/spans.csv. The files had until then been incorrect:

  • nodes.csv has a column nodes/0/location but it should have nodes/0/location/type and nodes/0/location/coordinates. This error is in the template/nodes.csv as well
  • spans.csv has spans/0/route but it should have spans/0/route/type and spans/0/route/coordinates. Again this error is in the template/spans.csv as well

The test is flagging the change as wrong due to a gitdiff but what manage.py pre-commit has done is corrected the files, it's unclear why my local manage.py has corrected these but the github auto version is generating the old wrong versions and so the gitdiff?

The documentation https://open-fibre-data-standard.readthedocs.io/en/latest/reference/publication_formats/csv.html#nodes is also incorrect when referencing location and route.

@duncandewhurst
Copy link
Collaborator

The CSV examples and documentation are correct and pre-commit is doing the right thing 🙂

The CSV format uses Well-Known Text (WKT) to represent geometries, per the examples in the introduction to the CSV format documentation, the description of Span.route and the description of Node.location:

In the JSON publication format, a GeoJSON Point geometry describing the physical location of this node. In the CSV publication format, a well-known text POINT geometry.

So in the CSV example files, nodes/0/location and spans/0/route are string literals, like POINT (-0.174 5.625), rather than being split into separate values for type and coordinates.

Support for WKT was added in Flatten Tool version 0.22 which is pinned in requirements.in so that is why the GitHub Actions workflow creates example files that use WKT format. I suspect that you have an earlier version of Flatten Tool installed in your local environment. You can check by running pip show flattentool. If your development environment is out of date, you can run pip-sync to update it.

Let me know if you're still running into problems.

@odscjen
Copy link
Collaborator Author

odscjen commented Jun 18, 2024

hmm, my flattentool version is 0.24.1. I followed all the instructions in https://ofds-standard-development-handbook.readthedocs.io/en/latest/technical/build.html for creating the dev environment. Using requirements.txt (as per the docs) gave me a load of errors

ERROR: Ignored the following versions that require a different python version: ...

I assume because my version of Ubuntu uses python 3.8, so I tried again using requirements.in in it's place in the relevant instruction step which ran fine with no errors reported.

When I ran manage.py I did get a message saying

/XXX/XXX/open-fibre-data-standard/.ve/lib/python3.8/site-packages/flattentool/json_input.py:382: UserWarning: Install flattentool's optional geo dependencies to use geo features.
warn(

From looking up the flatten-tool changelog I can see that that's about the WKT you're talking about. Incidently there's nothing in the flattentool documentation about how to install these optional geo dependencies but that's an issue for a different repo!

When I run pip-sync I get:

Traceback (most recent call last):
File "/usr/local/bin/pip-sync", line 5, in
from piptools.scripts.sync import cli
File "/usr/local/lib/python3.8/dist-packages/piptools/scripts/sync.py", line 11, in
from pip._internal.utils.misc import get_installed_distributions
ImportError: cannot import name 'get_installed_distributions' from 'pip._internal.utils.misc' (/home/jen/.local/lib/python3.8/site-packages/pip/_internal/utils/misc.py)

so that doesn't work.

I manually installed flatten-tool==0.22 and now it works and generates the correct csv example so I'll close this issue and make myself a note to always do this in the future.

@odscjen odscjen closed this as completed Jun 18, 2024
@duncandewhurst
Copy link
Collaborator

Good spot. Building the docs requires Python 3.9 or greater. I've added a note to the development handbook.

You should build from requirements.txt, not requirements.in, because it includes pinned versions of all dependencies so the build is reproducible. Of course you need to be on the correct version of Python for that to work!

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

No branches or pull requests

2 participants