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

679 Change of attributes for added individuals #698

Merged
merged 14 commits into from
Sep 8, 2021

Conversation

kysrpex
Copy link
Contributor

@kysrpex kysrpex commented Aug 26, 2021

See the linked issue for more details on the problem. The new behaviour will be the following for the next stable release that will be released from this hotfix branch.

rdflib 5.0.0

>>> from osp.core.namespaces import city, cuba
... from osp.core.utils import pretty_print
... import osp.core.warnings as warning_settings
... freiburg = city.City(name="Freiburg")
... industrial_area = city.Neighborhood(name = 'Industrie Gebiet', coordinates=[1, 8])
... print(industrial_area.coordinates)
... industrial_area.coordinates[0] = 98
... print(industrial_area.coordinates)
... industrial_area.coordinates = [98, 8]
... print(industrial_area.coordinates)
WARNING 2021-08-26 12:56:09,333 [osp.core]: Attribute city.coordinates references the mutable object [1 8] of type <class 'numpy.ndarray'>. Please note that if you modify this object in-place, the changes will not be reflected on the cuds object's attribute. 
For example, executing `fr = city.City(name='Freiburg', coordinates=[1, 2]); fr.coordinates[0]=98; fr.coordinates` would yield `array([1, 2])` instead of `array([98, 2])`, as you could expect. Use `fr.coordinates = [98, 2]` instead, or save the attribute to a different variable, i.e. `value = fr.coordinates; value[0] = 98, fr.coordinates = value`.
You will not see this kind of warning again during this session. You can turn off the warning by running `import osp.core.warnings as warning_settings; warning_settings.attributes_cannot_modify_in_place = False`.
[1 8]
[1 8]
[98  8]

A slightly different version of the warning will be merged to dev, since on dev you can upgrade to rdflib 6.0.0 given that you have the proper Python version installed.

rdflib 5.0.0

WARNING 2021-08-26 12:56:09,333 [osp.core]: Attribute city.coordinates references the mutable object [1 8] of type <class 'numpy.ndarray'>. Please note that because you have `rdflib < 6.0.0` installed,  if you modify this object in-place, the changes will not be reflected on the cuds object's attribute. 
For example, executing `fr = city.City(name='Freiburg', coordinates=[1, 2]); fr.coordinates[0]=98; fr.coordinates` would yield `array([1, 2])` instead of `array([98, 2])`, as you could expect. Use `fr.coordinates = [98, 2]` instead, or save the attribute to a different variable, i.e. `value = fr.coordinates; value[0] = 98, fr.coordinates = value`.
You will not see this kind of warning again during this session. You can turn off the warning by running `import osp.core.warnings as warning_settings; warning_settings.attributes_cannot_modify_in_place = False`.

On dev the dev branch, and with rdflib >= 6.0.0 installed, the user will see no warning, and in-place modification will work.

>>> from osp.core.namespaces import city, cuba
... from osp.core.utils import pretty_print
... import osp.core.warnings as warning_settings
... freiburg = city.City(name="Freiburg")
... industrial_area = city.Neighborhood(name = 'Industrie Gebiet', coordinates=[1, 8])
... print(industrial_area.coordinates)
... industrial_area.coordinates[0] = 98
... print(industrial_area.coordinates)
... industrial_area.coordinates = [98, 8]
... print(industrial_area.coordinates)
[1 8]
[98  8]
[98  8]

Upsides:

  • No bug.

Downsides:

  • Old code will not produce the same results, since it could be modifying things in-place. That's the reason behind the warning.

It's thus tricky to decide whether to define this as an incompatible API change (major version increase) or an error correction (patch version increase), as the in-place modification was a grey area. Maybe let's go for the error correction, given that the warning is there and makes it easy for the user to fix behaviour reliant on in-place modifications.

Edit: actually, the changes introduced to dev should increase the minor version number, we are promoting in-place modification from a grey area to an authorized feature, in a backwards-compatible way.


Closes #679. Merge first to master, merge also to dev (change the contents of setup.py to the appropriate ones after merging to master, restore the dev warning after merging to master). Since this is a critical fix, the package version was bumped.

kysrpex and others added 3 commits July 16, 2021 12:01
Authored-by: kysrpex <kysrpex@users.noreply.github.com>
Authored-by: José Manuel Domínguez <jose.manuel.dominguez@iwm.fraunhofer.de>
(cherry picked from commit 3251c71)

* OSP-core incompatible with RDFLib 6.0.0 (#678)
Authored-by: kysrpex <kysrpex@users.noreply.github.com>
Authored-by: José Manuel Domínguez <jose.manuel.dominguez@iwm.fraunhofer.de>
(cherry picked from commit fa6ebcd)

* Bump OSP-core version to 3.5.5 in hotfix branch for bugfix release.

Authored-by: kysrpex <kysrpex@users.noreply.github.com>
Authored-by: José Manuel Domínguez <jose.manuel.dominguez@iwm.fraunhofer.de>
@kysrpex kysrpex added 🐛 bug ⚠️ critical Such issues should be solved as soon as possible, and using a `hotfix branch`. 🛠️ challenging fix Likely to be solvable in at most a few days (full-time). labels Aug 26, 2021
@kysrpex kysrpex self-assigned this Aug 26, 2021
@kysrpex kysrpex changed the base branch from master to dev August 26, 2021 10:45
@kysrpex kysrpex marked this pull request as ready for review August 26, 2021 11:20
@kysrpex
Copy link
Contributor Author

kysrpex commented Aug 26, 2021

@pablo-de-andres @yoavnash You are seeing now what will be merged to master. After one of you approves, I'll merge to master. Then I will push commits for the dev version of this, request your review again, and merge to dev.

Copy link
Member

@yoavnash yoavnash left a comment

Choose a reason for hiding this comment

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

Move detailed warning to web page and refer to it by link.

@kysrpex
Copy link
Contributor Author

kysrpex commented Sep 1, 2021

Move detailed warning to web page and refer to it by link.

I started to do this and it fits in the CUDS API tutorial. However, the problem is that our readthedocs deployment does seem to render all docs versions. Thus, if somebody is using an old version of OSP-core, they will be linked to the latest version of the docs, which may show a different text.

I think fixing this aspect of readthedocs maybe is too much for an issue that is critical. We can do it later when this deployment thing is sorted out.

kysrpex added a commit that referenced this pull request Sep 1, 2021
Authored-by: kysrpex <kysrpex@users.noreply.github.com>
Authored-by: José Manuel Domínguez <jose.manuel.dominguez@iwm.fraunhofer.de>
(merged from 2d71d21)
@kysrpex
Copy link
Contributor Author

kysrpex commented Sep 2, 2021

I already merged this to master and released the hotfix. For dev, we can try to take the extra time.

@kysrpex kysrpex merged commit 80e8f5b into dev Sep 8, 2021
@kysrpex kysrpex deleted the 679-Change_of_attributes_for_added_individuals branch September 8, 2021 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug 🛠️ challenging fix Likely to be solvable in at most a few days (full-time). ⚠️ critical Such issues should be solved as soon as possible, and using a `hotfix branch`.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change of attributes for added individuals
2 participants