materialsproject / fireworks

The Fireworks Workflow Management Repo.
https://materialsproject.github.io/fireworks
Other
351 stars 184 forks source link

Fireworks broken with pymongo==4.0 #469

Closed utf closed 2 years ago

utf commented 2 years ago

Pymongo 4.0 removed support for the ssl_ca_certs connection option in favor of tlsCAFile. It was deprecated in 3.9 and now seems to have been removed (although the changelog does not explicitly state this). See: https://pymongo.readthedocs.io/en/stable/changelog.html?highlight=ssl_ca_certs#changes-in-version-3-9-0

This means LaunchPad objects cannot be initialised correctly.

Last working version is pymongo==3.12.2.

mkhorton commented 2 years ago

Specifically, for this block: https://github.com/materialsproject/fireworks/blob/f6f3165ccf2e14b7ec860f967ddac73fb4585c42/fireworks/core/launchpad.py#L232-L236

ssl=self.ssl,  # fine, though an alias for `tls`
ssl_ca_certs=self.ssl_ca_certs,  # now `tlsCAFile`
ssl_certfile=self.ssl_certfile,  # now `tlsCertificateKeyFile`
ssl_keyfile=self.ssl_keyfile,  # also now `tlsCertificateKeyFile`
ssl_pem_passphrase=self.ssl_pem_passphrase,  # now `tlsCertificateKeyFilePassword`

Would advise affected people to use uri_mode=True until a fix is released, since this should bypass the affected logic.

To fix, maybe something like the following in launchpad.py:

from importlib.metadata import version
pymongo_major_version = int(version("pymongo").split(".")[0])

if pymongo_major_version >= 4:
    self.connection = MongoClient(...)

Will submit a PR if I get a minute. FYI @computron think a few people affected by this

computron commented 2 years ago

If someone can submit a PR I will merge and release a new version

computron commented 2 years ago

@ardunn could you take a quick stab at this? Looks like it's causing widespread problems and Matt already sketched a decent skeleton solution, should be quick

ardunn commented 2 years ago

yes i will do now

ardunn commented 2 years ago

Ok after playing around with this in #471, having a conditional set of args kind of complicates things (including breaking FilePad tests, which would require a copy+paste of any code in LaunchPad). Also can break from_db_file and lots of other stuff.

@mkhorton what do you think about just lumping all these different arguments into mongoclient_kwargs? Then depending on version you should be able to pass in SSL params (if pymongo < 4.0) or TLS params (if pymongo >= 4.0). This also should allow for existing launchpad scripts to still(?) work, since the ssl/tls args will get passed in as mongoclient_kwargs.

ardunn commented 2 years ago

Another option is forcing pymongo upgrade to >=4.0 in the requirements and just changing the arguments. But supporting both explicitly seems like trouble

mkhorton commented 2 years ago

I think you're right that the better option is to just require >= 4.0 rather than supporting both.

I agree that explicitly specifying each kwarg will likely lead to further breakages in the future when MongoClient is updated with a future version, but perhaps removing the explicit kwargs now also might break code elsewhere too?

The additional problem here is that the current released version is not pinned to the major version -- it specifies "pymongo>=3.3.0" rather than "pymongo>=3.3.0<4.0", meaning that people might run into this issue for a while regardless since version solvers will expect pymongo 4.x to work.

ardunn commented 2 years ago

@computron this is not resolved