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

Flask-PyMongo 2.0 #110

Closed
dcrosta opened this Issue May 19, 2018 · 20 comments

Comments

Projects
None yet
3 participants
@dcrosta
Owner

dcrosta commented May 19, 2018

TL;DR:

  • Flask-PyMongo 2.0.0b1 is available on PyPI (use pip install --pre ...; docs here)
  • It is not backwards incompatible. You must use a URI to configure it (app.config["MONGO_URI"] = ...)
  • Tentative release date July 1, 2018
    ** If you don't have time to update your app to use a URI by then, pin down to version 0.5.2 in your requirements

Keeping up with PyMongo and MongoDB developments has been a continual struggle for me as maintainer of Flask-PyMongo, as attested to by the long list of bugs filed. I have let down those who use this project, and I apologize for that.

In order to make the maintenance load acceptable, I have decided to release version 2.0 which will remove all of the "split out" configuration functionality. In 2.0, Flask-PyMongo will only read a MongoDB connection URI from Flask config. For cases where some configuration cannot be passed via URI, it will also accept keyword arguments, which will be passed directly to PyMongo.

I anticipate that this will require changes from many users of Flask-PyMongo, and I apologize for the disruption. I believe that this direction will enable me to keep up with compatibility requests, reduce the number of custom forked versions, and provide a more compact and stable platform for future development.

If you have not yet done so, please pin your version of Flask-PyMongo to the current version (e.g. flask-pymongo<2.0 in a requirements.txt file).

I will try to answer any questions that may come up in this thread until (and likely slightly after) the release of 2.0.

@hbldh

This comment has been minimized.

Show comment
Hide comment
@hbldh

hbldh May 23, 2018

Contributor

When in time do you see a version 2.0 being ready for testing?
Also, since I use flask-pymongo for several projects I would also like to offer my help in making version 2.0 a reality.

Contributor

hbldh commented May 23, 2018

When in time do you see a version 2.0 being ready for testing?
Also, since I use flask-pymongo for several projects I would also like to offer my help in making version 2.0 a reality.

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta May 23, 2018

Owner

I hope to have an RC ready some time in June. Unlike with past releases, I'm planning to do at least one rc release before 2.0, to let folks have a chance to experiment with it and get some feedback. That might add another month to the cycle, just to ensure that we've had an adequate chance to find any major issues before cutting 2.0.

Owner

dcrosta commented May 23, 2018

I hope to have an RC ready some time in June. Unlike with past releases, I'm planning to do at least one rc release before 2.0, to let folks have a chance to experiment with it and get some feedback. That might add another month to the cycle, just to ensure that we've had an adequate chance to find any major issues before cutting 2.0.

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta May 24, 2018

Owner

For anyone interested in following along with development, I've started a branch https://github.com/dcrosta/flask-pymongo/tree/two-point-oh

Owner

dcrosta commented May 24, 2018

For anyone interested in following along with development, I've started a branch https://github.com/dcrosta/flask-pymongo/tree/two-point-oh

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta May 28, 2018

Owner

@hbldh and anyone else here, if you want to live on the bleeding bleeding edge, there's now a probably-working version on branch two-point-oh. I hope to push that, or something substantially like it, to PyPI as 2.0.0b1 within the next week, I just need to convince myself a bit more that it works. (Feedback from others will help speed this along, of course!)

Owner

dcrosta commented May 28, 2018

@hbldh and anyone else here, if you want to live on the bleeding bleeding edge, there's now a probably-working version on branch two-point-oh. I hope to push that, or something substantially like it, to PyPI as 2.0.0b1 within the next week, I just need to convince myself a bit more that it works. (Feedback from others will help speed this along, of course!)

@hbldh

This comment has been minimized.

Show comment
Hide comment
@hbldh

hbldh May 30, 2018

Contributor

Very nice! I will try to test it in a few of my apps over the next few days.

Contributor

hbldh commented May 30, 2018

Very nice! I will try to test it in a few of my apps over the next few days.

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta May 30, 2018

Owner

@hbldh it's on PyPI as a beta now (https://pypi.org/manage/project/Flask-PyMongo/release/2.0.0b1/) so pip install --pre flask-pymongo should get you what you want.

Updated docs at https://flask-pymongo.readthedocs.io/en/2.0.0b1/

Owner

dcrosta commented May 30, 2018

@hbldh it's on PyPI as a beta now (https://pypi.org/manage/project/Flask-PyMongo/release/2.0.0b1/) so pip install --pre flask-pymongo should get you what you want.

Updated docs at https://flask-pymongo.readthedocs.io/en/2.0.0b1/

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta Jun 6, 2018

Owner

Hey @hbldh -- just wondering if you've had a chance to try 2.0 out, and if you have any feedback?

Owner

dcrosta commented Jun 6, 2018

Hey @hbldh -- just wondering if you've had a chance to try 2.0 out, and if you have any feedback?

@hbldh

This comment has been minimized.

Show comment
Hide comment
@hbldh

hbldh Jun 8, 2018

Contributor

I have now tried it out - at long last due to illness - in a few of my apps, and 2.0.0b1 works without any modification at all in my apps; a lovely feeling indeed! Since they are mostly deployed on Heroku, they were all provisioned by MONGO_URI environment variable and therefore predisposed towards the only remaining way of configuring a PyMongo client, but anyway. Nice work!

The code is commendably slim in my opinion. The only thing I have thought about is whether the
register_bson_object_id_converter will ever be called by a user or if it could be an internal method just to make the API even more clean.

Have you tried monkeypatching the true PyMongo Collection object with a find_one_or_404 method instead of using the wrapper structure? It might lead to even less code to manage and using PyMongo's own objects to an even greater extent, but the code could be perceived as more cryptic and unreadable. Not sure if it is a good idea...

Contributor

hbldh commented Jun 8, 2018

I have now tried it out - at long last due to illness - in a few of my apps, and 2.0.0b1 works without any modification at all in my apps; a lovely feeling indeed! Since they are mostly deployed on Heroku, they were all provisioned by MONGO_URI environment variable and therefore predisposed towards the only remaining way of configuring a PyMongo client, but anyway. Nice work!

The code is commendably slim in my opinion. The only thing I have thought about is whether the
register_bson_object_id_converter will ever be called by a user or if it could be an internal method just to make the API even more clean.

Have you tried monkeypatching the true PyMongo Collection object with a find_one_or_404 method instead of using the wrapper structure? It might lead to even less code to manage and using PyMongo's own objects to an even greater extent, but the code could be perceived as more cryptic and unreadable. Not sure if it is a good idea...

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta Jun 8, 2018

Owner

Awesome, that's exciting good news!

I split out register_bson_object_id_converter thinking that maybe people would want just that, but might use PyMongo directly. But I suppose they could do the one line that that method does by themselves. I don't feel very strongly about it, which is probably reason to remove it (easier to add later than remove). If anyone does feel strongly, speak up.

Re: monkeypatching, I'd rather not. It probably could be done relatively safely, but it feels wrong, and the wrappers don't add any overhead other than in db.foo.bar-like expressions. I'm inclined to leave it as is.

Owner

dcrosta commented Jun 8, 2018

Awesome, that's exciting good news!

I split out register_bson_object_id_converter thinking that maybe people would want just that, but might use PyMongo directly. But I suppose they could do the one line that that method does by themselves. I don't feel very strongly about it, which is probably reason to remove it (easier to add later than remove). If anyone does feel strongly, speak up.

Re: monkeypatching, I'd rather not. It probably could be done relatively safely, but it feels wrong, and the wrappers don't add any overhead other than in db.foo.bar-like expressions. I'm inclined to leave it as is.

@hbldh

This comment has been minimized.

Show comment
Hide comment
@hbldh

hbldh Jun 9, 2018

Contributor

The register_bson... is not something I feel strongly about, so leave it as is.
As for the monkeypatch I agree with your assessment.

However I remembered one thing: #87 is a very valid point, made by a MongoDB employee, and I think connect should default to False in case nothing is specified in URI. It is the only option that should be handled separately due to fork safety.

Contributor

hbldh commented Jun 9, 2018

The register_bson... is not something I feel strongly about, so leave it as is.
As for the monkeypatch I agree with your assessment.

However I remembered one thing: #87 is a very valid point, made by a MongoDB employee, and I think connect should default to False in case nothing is specified in URI. It is the only option that should be handled separately due to fork safety.

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta Jun 9, 2018

Owner

I'd like to hew as close as possible to PyMongo's defaults, so I don't think we should synthesize arguments (that was the whole point of 2.0, to stop doing that!), but since most people using Flask are probably using some kind of forking, I'll add a note to the documentation.

Owner

dcrosta commented Jun 9, 2018

I'd like to hew as close as possible to PyMongo's defaults, so I don't think we should synthesize arguments (that was the whole point of 2.0, to stop doing that!), but since most people using Flask are probably using some kind of forking, I'll add a note to the documentation.

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta
Owner

dcrosta commented Jun 9, 2018

(See d50af87)

@hbldh

This comment has been minimized.

Show comment
Hide comment
@hbldh

hbldh Jun 11, 2018

Contributor

I understand your aim to not introduce any non-pymongo defaults and I think it is a very good idea, in general.

However, given that this project caters to web applications and web applications have a tendency to run into this issue if a pre-fork server is used, I would like to argue that there is such a strong reason to have one exception to the "do not synthesize arguments" rule, namely connect=False if nothing else is specified.

Yes, adding it as a keyword argument in the creation of the Flask-PyMongo client is very simple, as is adding it to the URI. Having a note in the documentation about how to resolve potential issues is great, but just having it set to False by default could possibly preemptively solve the problem for some users.
Furthermore, it is not a very disruptive setting that gets in the user's way if set to False without their knowledge. It will merely transfer the connect delay from app start to first PyMongo-client use.

Anyway, at least I have made a plea for my case now.

Contributor

hbldh commented Jun 11, 2018

I understand your aim to not introduce any non-pymongo defaults and I think it is a very good idea, in general.

However, given that this project caters to web applications and web applications have a tendency to run into this issue if a pre-fork server is used, I would like to argue that there is such a strong reason to have one exception to the "do not synthesize arguments" rule, namely connect=False if nothing else is specified.

Yes, adding it as a keyword argument in the creation of the Flask-PyMongo client is very simple, as is adding it to the URI. Having a note in the documentation about how to resolve potential issues is great, but just having it set to False by default could possibly preemptively solve the problem for some users.
Furthermore, it is not a very disruptive setting that gets in the user's way if set to False without their knowledge. It will merely transfer the connect delay from app start to first PyMongo-client use.

Anyway, at least I have made a plea for my case now.

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta Jun 11, 2018

Owner

@ajdavis hoping you (or one of your co-worker more currently involved in PyMongo, perhaps?) can shed some light here -- a) is forking still an issue in modern versions of pymongo, and b) if it's problematic, why still do it (ie default to connect=True)?

@hbldh I'm still pretty wary of going down the path of adding defaults. At minimum, I'd like to hear from some more users that this is causing problems or inconvenient for them before making a call here.

Owner

dcrosta commented Jun 11, 2018

@ajdavis hoping you (or one of your co-worker more currently involved in PyMongo, perhaps?) can shed some light here -- a) is forking still an issue in modern versions of pymongo, and b) if it's problematic, why still do it (ie default to connect=True)?

@hbldh I'm still pretty wary of going down the path of adding defaults. At minimum, I'd like to hear from some more users that this is causing problems or inconvenient for them before making a call here.

@ajdavis

This comment has been minimized.

Show comment
Hide comment
@ajdavis

ajdavis Jun 15, 2018

a) Yes, forking is still an issue. It's basically impossible in any programming language including Python to design a class that starts threads and opens connections before a fork, and is still in a correct state afterward. (More info: PYTHON-1139, PYTHON-1348.)

b) If we defaulted connect=False, that would help people who do exactly this:

client = MongoClient()
fork()
# Trigger connection by doing any operation.
client.db.collection.insert_one({})

But it doesn't save anyone who does this:

client = MongoClient()
# Trigger connection by doing any operation.
client.db.collection.insert_one({})
fork()

Since that's a bit subtle, we decided that our current connect=True default has a more reliable behavior: if you create a client before you fork, it warns you when you use it after the fork.

The best solution IMO for all users is to create the client after the fork. However, if they pass connect=False and carefully avoid using the client before the fork then it will work. If you think that Flask-PyMongo users are likely to understand the distinction and will benefit from a better default, then defaulting connect=False here could be worthwhile.

ajdavis commented Jun 15, 2018

a) Yes, forking is still an issue. It's basically impossible in any programming language including Python to design a class that starts threads and opens connections before a fork, and is still in a correct state afterward. (More info: PYTHON-1139, PYTHON-1348.)

b) If we defaulted connect=False, that would help people who do exactly this:

client = MongoClient()
fork()
# Trigger connection by doing any operation.
client.db.collection.insert_one({})

But it doesn't save anyone who does this:

client = MongoClient()
# Trigger connection by doing any operation.
client.db.collection.insert_one({})
fork()

Since that's a bit subtle, we decided that our current connect=True default has a more reliable behavior: if you create a client before you fork, it warns you when you use it after the fork.

The best solution IMO for all users is to create the client after the fork. However, if they pass connect=False and carefully avoid using the client before the fork then it will work. If you think that Flask-PyMongo users are likely to understand the distinction and will benefit from a better default, then defaulting connect=False here could be worthwhile.

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta Jun 16, 2018

Owner

OK so I'm pretty convinced connect=False is the right behavior. (I would argue that PyMongo ought to do that, even though it doesn't save you in all cases, but I gather that you all have been through that on your side already).

It looks like right now, keyword arguments take precedence over the same value specified as a param in the connection URL. Is that guaranteed always to be true? (ie if I say connect=False as a kwarg, and URL is "mongodb://localhost/test?connect=true"?)

Owner

dcrosta commented Jun 16, 2018

OK so I'm pretty convinced connect=False is the right behavior. (I would argue that PyMongo ought to do that, even though it doesn't save you in all cases, but I gather that you all have been through that on your side already).

It looks like right now, keyword arguments take precedence over the same value specified as a param in the connection URL. Is that guaranteed always to be true? (ie if I say connect=False as a kwarg, and URL is "mongodb://localhost/test?connect=true"?)

@ajdavis

This comment has been minimized.

Show comment
Hide comment
@ajdavis

ajdavis Jun 16, 2018

Keywords beat URL parameters until PyMongo 4.0 at least, since changing it would be a BC break. I think it's the right precedence anyway and will never change.

ajdavis commented Jun 16, 2018

Keywords beat URL parameters until PyMongo 4.0 at least, since changing it would be a BC break. I think it's the right precedence anyway and will never change.

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta Jun 19, 2018

Owner

OK, I'll make the change. It seems at worst to just change when a potentially-slow operation takes place, and at best to save a few people in a few situations from unexpected breakage. Thanks for keeping me honest @hbldh and for your advice @ajdavis!

Owner

dcrosta commented Jun 19, 2018

OK, I'll make the change. It seems at worst to just change when a potentially-slow operation takes place, and at best to save a few people in a few situations from unexpected breakage. Thanks for keeping me honest @hbldh and for your advice @ajdavis!

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta Jun 19, 2018

Owner

Flask-PyMongo 2.0.0b2 now available on PyPI (install with pip install --pre Flask-PyMongo).

Owner

dcrosta commented Jun 19, 2018

Flask-PyMongo 2.0.0b2 now available on PyPI (install with pip install --pre Flask-PyMongo).

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta Jul 2, 2018

Owner

I've just released Flask-PyMongo 2.0 to PyPI.org, so I'm closing this now. As always, please report any issues back here.

Owner

dcrosta commented Jul 2, 2018

I've just released Flask-PyMongo 2.0 to PyPI.org, so I'm closing this now. As always, please report any issues back here.

@dcrosta dcrosta closed this Jul 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment