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

Mark ConjunctiveGraph as deprecated #2405

Open
aucampia opened this issue May 22, 2023 · 7 comments
Open

Mark ConjunctiveGraph as deprecated #2405

aucampia opened this issue May 22, 2023 · 7 comments
Labels
breaking change This involves or proposes breaking RDFLib's public API. concept: RDF dataset Relates to the RDF datasets concept. core Relates to core functionality of RDFLib, i.e. `rdflib.{graph,store,term}` good first issue Good for newcomers

Comments

@aucampia
Copy link
Member

ConjunctiveGraph is not a concept defined by the W3C spec, and it is not clear that it is a needed concept. People should use Dataset instead, as that is a much more well-defined concept.

If we intend to get rid of ConjunctiveGraph, then we should add a deprecation notice that it will be removed in the next major release.

Subsequently, we should also remove its use in the codebase where it occurs. Though, best to first mark it as deprecated.

@aucampia aucampia added backwards incompatible will affect backwards compatibility, this includes things like breaking our interface good first issue Good for newcomers breaking change This involves or proposes breaking RDFLib's public API. core Relates to core functionality of RDFLib, i.e. `rdflib.{graph,store,term}` concept: RDF dataset Relates to the RDF datasets concept. and removed backwards incompatible will affect backwards compatibility, this includes things like breaking our interface labels May 22, 2023
@ghost
Copy link

ghost commented May 22, 2023

ConjunctiveGraph is not a concept defined by the W3C spec, and it is not clear that it is a needed concept. People should use Dataset instead, as that is a much more well-defined concept.

If we intend to get rid of ConjunctiveGraph, then we should add a deprecation notice that it will be removed in the next major release.

Subsequently, we should also remove its use in the codebase where it occurs. Though, best to first mark it as deprecated.

Bit of a chicken and egg situation. In a whole host pf places, until Dataset is working as per the spec (i.e. context-as-id implemented), it can't be used freely as a replacement for ConjunctiveGraph. An initial step might be to replace its use in internal calls such as the parsers.

@aucampia
Copy link
Member Author

aucampia commented May 22, 2023

Subsequently, we should also remove its use in the codebase where it occurs. Though, best to first mark it as deprecated.

Bit of a chicken and egg situation. In a whole host pf places, until Dataset is working as per the spec (i.e. context-as-id implemented), it can't be used freely as a replacement for ConjunctiveGraph. An initial step might be to replace its use in internal calls such as the parsers.

Fair point, but I guess the steps are then:

  1. Mark ConjunctiveGraph as deprecated to signal to people that it will be removed in future versions.
  2. Fix Dataset to be generally usable by incrementally addressing specific problems with it that make it non-compliant with the spec.
  3. Once Dastaset is usable, replace ConjunctiveGraph with Dataset.

Though it is also important to note, ConjunctiveGraph is not working as per the spec either, as it is not defined in the spec. So if we have two things not working according to the spec, and we know we want to get rid of one, then it may still make sense to move code that uses the one to the other.

A subsequent PR may still break that code, but breaking changes is not something that we can avoid, so anticipation of potential breaking changes should not necessarily block other changes.

@ghost
Copy link

ghost commented May 26, 2023

Some “gotchas”: Dataset subclasses ConjunctiveGraph and so does IsoMorphicGraph and then this unhappily propagates through to graph_diff via ReadOnlyGraphAggregate

Fix Dataset to be generally usable by incrementally addressing specific problems with it that make it non-compliant with the spec.

A fair amount of the non-compliance arises from its subclassing of ConjunctiveGraph. I have a horrible suspicsion that we will need to go backwards a bit first by duplicating / adapting / correcting the ConjunctiveGraph methods and then having it subclass from Graph. Once that's in place, Dataset will be completety decoupled from ConunctiveGraph and can replace ConjunctiveGraph uses in the internal code and the latter's use deprecated.

@mielvds
Copy link
Contributor

mielvds commented Jun 7, 2023

Can we already make a finite list of issues that would lead to W3C spec compliance for Dataset? You have https://github.com/RDFLib/rdflib/issues?q=is%3Aissue+is%3Aopen+label%3A%22concept%3A+RDF+dataset%22, but that has a wider scope.

And I agree with @gjhiggins about changing the parent class first, but I don't think it will be that horrible :)

@aucampia
Copy link
Member Author

aucampia commented Jun 8, 2023

Can we already make a finite list of issues that would lead to W3C spec compliance for Dataset?

This is harder than it would seem. We run the W3C test suites and there are some tests that fail, and they are all marked with xfail (see this for example). But for the most part the tests relating to Dataset pass. The one PR that fixes one of the most serious problems with Dataset IMO, #2406, does not eliminate any of the xfails. This is also a very controversial PR and has 0 reviews and 0 approvals so far. The reason why this is both a contraversial PR and why it does not eliminate XFails is because there are various hacks in place to work around it in the tests so it is possible to do some testing at least.

I don't think an effort to document all issues will be helpful. We could do better at categorizing errors, I think if you ask @nicholascar or @gjhiggins to give you permission to triage issues it can also help. I think we should have a label for W3C spec compliance issues, but I think the best we can do is categorize existing issues in a best effort manner and try and fix concrete problems in the code base of which there is a lot.

And I agree with @gjhiggins about changing the parent class first, but I don't think it will be that horrible :)

It won't be horrible, but at the rate of improvement it will take some years. When someone writes code with RDFLib today, they should not be using ConjunctiveGraph. They should be using Dataset. I know Dataset has issues, but there are almost 200 open issues against RDFLib and they are just scratching the surface I think. We don't want to keep fixing ConjunctiveGraph, we want to fix Dataset. If we have to fix ConjunctiveGraph to fix Dataset, then we have to do that, but even if we fix all issues with ConjunctiveGraph new code should still not use it. Whether Dataset is backed by ConjunctiveGraph does not change what users of RDFLib should do, which is use Dataset instead.

@mielvds
Copy link
Contributor

mielvds commented Sep 13, 2023

I had a thought about this: would it be possible to already reverse the dependency between CG and dataset, ie. make ConjunctiveGraph a subclass of Dataset and retain all of the current semantics? It wouldnt change much, but perhaps you can start fixing things in Dataset and deprecate CG more easily. Another idea would be to duplicate the code and make them independent of eachother.

@aucampia
Copy link
Member Author

We could deprecate CG fairly easily now, it would require some odd things, like only raising a deprecation warning based on type of self, but it won't be too challenging.

I just have not gotten round to it yet.

wallberg added a commit to wallberg/rdflib that referenced this issue May 12, 2024
wallberg added a commit to wallberg/rdflib that referenced this issue May 12, 2024
nicholascar added a commit that referenced this issue Jun 17, 2024
* Deprecate ConjunctiveGraph (#2405)

* docs: deprecate ConjunctiveGraph (#2405)

---------

Co-authored-by: Nicholas Car <nick@kurrawong.net>
minhnh added a commit to minhnh/rdf-utils that referenced this issue Nov 12, 2024
- move python.py and models.py to a models directory
- update unit tests
- use Dataset instead of ConjunctiveGraph per RDFLib/rdflib#2405
minhnh added a commit to secorolab/rdf-utils that referenced this issue Nov 15, 2024
- move python.py and models.py to a models directory
- update unit tests
- use Dataset instead of ConjunctiveGraph per RDFLib/rdflib#2405
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This involves or proposes breaking RDFLib's public API. concept: RDF dataset Relates to the RDF datasets concept. core Relates to core functionality of RDFLib, i.e. `rdflib.{graph,store,term}` good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants