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

Dependencies: update to flask~=2.0 for rest extra #5366

Merged
merged 1 commit into from
Feb 18, 2022

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Feb 18, 2022

This major version of flask has been out for close to a year and
our docs build started failing due to itsdangerous having removed
the json module that the old flask was still using

Propose to supdersede #5365

@chrisjsewell
Copy link
Member

chrisjsewell commented Feb 18, 2022

https://itsdangerous.palletsprojects.com/en/2.1.x/changes/#version-2-0-0

Simplejson is no longer used if it is installed. To use a different library, pass it as Serializer(serializer=...)

This seems relevant, since we use simplejson for encoding/decoding to the database. Can you check

@@ -33,7 +33,7 @@ deprecation==2.1.0
disk-objectstore==0.6.0
docutils==0.15.2
entrypoints==0.3
Flask==1.1.2
Flask==2.0.0
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest using 2.1.0, since that is the latest release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only see 2.03 on PyPI

Copy link
Member

Choose a reason for hiding this comment

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

Yeh that's ok, I was confusing myself with the itsdangerous version.
I think that version should also be updated here though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@chrisjsewell
Copy link
Member

This is the corresponding change in flask:

JSON support no longer uses simplejson. To use another JSON module, override app.json_encoder and json_decoder. :issue:3555

https://github.com/pallets/flask/blob/main/CHANGES.rst

@sphuber
Copy link
Contributor Author

sphuber commented Feb 18, 2022

Looking at [552c626] the use of simplejson was added to provide consistency between Python 2 and Python 3. I would actually be tempted to investigate if this is still needed and actually try to drop the requirement and use the built-in json package. (Edit: since itsdangerous also drops Python 2 support in v2.0, they probably had a similar reason for having a custom json implementation)

But that is irrelevant for this change in this PR I think. Flask is only used for the REST API and we shouldn't care if they use a different JSON package underneath, do we?

@sphuber sphuber force-pushed the fix/dependency-flask branch from be3a413 to e976254 Compare February 18, 2022 09:15
@sphuber sphuber force-pushed the fix/dependency-flask branch from e976254 to d81ddff Compare February 18, 2022 09:16
@chrisjsewell
Copy link
Member

if this is still needed and actually try to drop the requirement and use the built-in json package.

I believe there may be differences in encoding certain things like numpy nan/inf and Decimal (see e.g. #4964)

@sphuber
Copy link
Contributor Author

sphuber commented Feb 18, 2022

I believe there may be differences in encoding certain things like numpy nan/inf and Decimal (see e.g. #4964)

I don't think aiida.common.json is involved in this process of serializing node content to the database though. The attributes and extras are passed through aiida.orm.implementation.utils.clean_value but that doesn't actually do JSON serialization directly. It does a manual conversion of certain types and the final serialization to JSON is left to the backend, so PostgreSQL in our case through the JSONB fields. The aiida.common.json is really only used by AiiDA when writing files to disk, or the file repository.

@chrisjsewell
Copy link
Member

I don't think aiida.common.json is involved in this process of serializing node content to the database though

return create_engine(
engine_url,
json_serializer=json.dumps,
json_deserializer=json.loads,
future=True,
encoding='utf-8',
**config.get('engine_kwargs', {}),
)

@sphuber
Copy link
Contributor Author

sphuber commented Feb 18, 2022

I don't think aiida.common.json is involved in this process of serializing node content to the database though

return create_engine(
engine_url,
json_serializer=json.dumps,
json_deserializer=json.loads,
future=True,
encoding='utf-8',
**config.get('engine_kwargs', {}),
)

I stand corrected 😅 Still point is not relevant for this discussion on flask and its use in the REST API, is it?

@chrisjsewell
Copy link
Member

Still point is not relevant for this discussion on flask and its use in the REST API, is it?

No idea 🤷‍♂️, just wanted to note it as a possible source of change, e.g. if something gets deserialised from the database, with simplejson, then flask tries to "re-serialise" it with json.
Again though, I'm not particularly precious about the rest code, because I want to eventually completely replace it anyway (with aiida-restapi)

@sphuber sphuber requested a review from chrisjsewell February 18, 2022 09:50
@sphuber sphuber merged commit 2937ff1 into aiidateam:develop Feb 18, 2022
@sphuber sphuber deleted the fix/dependency-flask branch February 18, 2022 09:53
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

Successfully merging this pull request may close these issues.

2 participants