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.

Sign up for GitHub

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

Jump to bottom

Update requirements #212

Closed
wants to merge 8 commits into from
Closed

Update requirements #212

wants to merge 8 commits into from

Conversation

CasperWA
Copy link
Contributor

@CasperWA CasperWA commented Sep 15, 2021 •
edited

This does not close the #206 issue anymore. See the comment below.

Set version lofts for Python dependencies.

@CasperWA CasperWA mentioned this pull request Sep 15, 2021
Closed
francescalb
francescalb requested changes Sep 15, 2021
View reviewed changes
Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

Unless there is a specific reason not to accept rdflib5.0.0 and up I still think we should allow that.
The more restrictive we are the higher chance of not being compatible with other packages for no good reason.

@CasperWA
Copy link
Contributor Author

CasperWA commented Sep 15, 2021 •
edited

Unless there is a specific reason not to accept rdflib5.0.0 and up I still think we should allow that.
The more restrictive we are the higher chance of not being compatible with other packages for no good reason.

Under normal circumstances I'd agree, but since these releases are only a couple of months apart, I don't think there will be a huge difference. Also, there are no releases between version 5.0.0 and 6.0.0. RDFLib releases: https://github.com/RDFLib/rdflib/releases and https://pypi.org/project/rdflib/#history

Furthermore, if we allowed version 5.0.0, the issue mentioned in #206 would not be resolved.

@francescalb
Copy link
Collaborator

francescalb commented Sep 16, 2021 via email

@CasperWA
Copy link
Contributor Author

5.0.0 resolves the issue206, it is 4.x.x that makes that error as far as I understand. If there is little difference between 5.0.0 and 6.0.0 there is even less reason to be restrictive.

Again, as far as I read the rdflib changelog, this is not the case.
Changelog: https://github.com/RDFLib/rdflib/blob/master/CHANGELOG.md
The issue from the RDFLib repository in question (referenced in the "Known issues" list in the EMMO-python readme): RDFLib/rdflib#1043.

It seems from the changelog that this was fixed by PR RDFLib/rdflib#1054, which was merged after 5.0.0 and only included in 6.0.0.
This also matches up with the "Known issues" text that "This issue was fixed after the release of rdflib 5.0.0".

@francescalb
Copy link
Collaborator

francescalb commented Sep 16, 2021 via email

@CasperWA
Copy link
Contributor Author

Concerning the difference between 5.0.0 and 6.0.0, from the changelog it seems there are many vital improvements. I don't see why we should support an older version with several bugs that have already been fixed in a newer version while also considering that the number of repositories that only support version 5.0.0 and not 6.0.0 have only had a 2 month window to do so.

The only thing that might be "dangerous" is that version 6.0.0 drops Python 2 and Python 3 < 3.7 support. I.e., this repository should also drop Python 3.6 support if the minimum rdflib version should be 6.0.0.

@jesper-friis
Copy link
Collaborator

I think that rdflib has provided the features we use since v2.0 or maybe v3.0. Most linux distributions do not provide rdflib v6. Fedora 33 (which was released this spring) is e.g. on version 4.2.1. Requiring a higher version of rdflib that we actually use may unnecessary enforce people to use a virtual environment instead of the version of rdflib that comes with their distribution.

I am not sure we want to go down this road - Emmontopy is after all a fairly simple package.

@CasperWA
Copy link
Contributor Author

I think that rdflib has provided the features we use since v2.0 or maybe v3.0. Most linux distributions do not provide rdflib v6. Fedora 33 (which was released this spring) is e.g. on version 4.2.1. Requiring a higher version of rdflib that we actually use may unnecessary enforce people to use a virtual environment instead of the version of rdflib that comes with their distribution.

Ah - excellent point.

I'll add back the support for earlier versions 👍

We'll keep the issue open then, and I'll try to add some version-aware code around the functionality that's affected by issue #206.

@CasperWA
Set rdflib min version to 6.0.0
1bf2c7f 
Since 6.0.0 is the immediate release after 5.0.0, this makes sense. And
it's nicer to have `>=` dependencies than `>`.

Note, there is an issue with 5.0.0 that produces wrong turtle files.
@CasperWA
Remove known issue note about rdflib
0f72d9d
@CasperWA
Update requirements.txt with version lofts
1d0d61a
@CasperWA
Support latest rdflib package on Linux distros
38c456d
@CasperWA
Check rdflib version is >=6.0.0 for turtle output
a836428 
An error will be thrown if this is not satisfied.
This is also true for the `ontoconvert` CLI tool.
@CasperWA
Copy link
Contributor Author

I have added some sanity checks for having rdflib be version 6.0.0 or higher if one wants to output Turtle files using the ontoconvert CLI tools and connected utility Python functions.

I don't know if this is the desired result, or if it should instead be a warning and then continue?

@CasperWA
Copy link
Contributor Author

CasperWA commented Sep 17, 2021 •
edited

I think that rdflib has provided the features we use since v2.0 or maybe v3.0. Most linux distributions do not provide rdflib v6. Fedora 33 (which was released this spring) is e.g. on version 4.2.1. Requiring a higher version of rdflib that we actually use may unnecessary enforce people to use a virtual environment instead of the version of rdflib that comes with their distribution.

I am not sure we want to go down this road - Emmontopy is after all a fairly simple package.

As a further note to this, I'd always recommend installing Python packages in virtual environments - but I think it's good to support this use case.

This was referenced Sep 20, 2021
Closed
Merged
@CasperWA
Copy link
Contributor Author

This has been superseded by #216

@CasperWA CasperWA closed this Sep 21, 2021
@CasperWA CasperWA deleted the close-206-required-rdf-5 branch September 21, 2021 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jesper-friis jesper-friis Awaiting requested review from jesper-friis

@francescalb francescalb Awaiting requested review from francescalb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@CasperWA @francescalb @jesper-friis