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

Add dependency on omero-py 5.8+ #45

Closed
wants to merge 1 commit into from
Closed

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Sep 23, 2020

The primary motivation is to get rid of the DeprecationWarning thrown on every invocation of omero render set Object:id file.yml fixed in ome/omero-py@dcde6b2.

Also remove unnecessary PyYAML dependency which should be declared transitively

Proposed tag 0.6.2

Remove unnecessary PyYAML dependency which should be declared transitively
@joshmoore
Copy link
Member

I don't have any objections, but this isn't strictly necessary, right? It's still valid to use omero-cli-render with omero-py<5.8, no?

@manics
Copy link
Member

manics commented Sep 24, 2020

This effectively makes the dependencies of omero-py part of the OMERO.py API. For instance if we were to strip out all the old Python 2 compatibility code from OMERO.py it wouldn't need future but we'd have to keep it otherwise this plugin and others might break. If we wanted to change an OMERO.py method implementation to use a different library (e.g. an alternative YAML implementation) we'd still have to keep the old one.

If this is something we want can we document it in the OMERO.py docs, e.g. is it all dependencies, just YAML/future, does bumping a major version of an OMERO.py dependency count as a breaking OMERO.py release, etc.

@sbesson
Copy link
Member Author

sbesson commented Sep 24, 2020

Sorry, my description was way incomplete - updated now. The dependency cleanup is a by-product of this PR - happy to revert it.

The primary motivation was to consume the changes in ome/omero-py@dcde6b2 and reduce the warnings thrown on every invocation of the commands.

@joshmoore
Copy link
Member

The primary motivation was to ... reduce the warnings thrown on every invocation of the commands.

Understood, but assuming there were a bug in 5.8 this would make it difficult for users to rollback, no?

For instance if we were to strip out all the old Python 2 compatibility code from OMERO.py it wouldn't need future but we'd have to keep it otherwise this plugin and others might break.

I'm not sure I follow. It seems like only the case where omero-py didn't have a library specified, would require changes to this downstream repo:

  • if we add a library to omero-py, then installing that new version will pull in the repo, so this repo is happy.
  • if we remove a library from omero-py, then this repo doesn't need (and even if it's defined then that won't hurt anyone)

I'm not saying there won't be situations where we will need to force a hard bump, but this isn't one of them, correct?

@manics
Copy link
Member

manics commented Sep 24, 2020

It affects future maintainance of OMERO.py and all dependent projects. For example if we remove future and yaml from OMERO.py in future is that a breaking change to OMERO.py? If is then we either have to keep those old libraries in install_requires or make a major release. If it's not then we should document in our developer docs that although you can use OMERO.py dependencies in your own plugins we may remove or change them in a non-breaking release.

Since this is one of our own plugins we should follow whatever our "best practice" is, which we need to agree on- is it OK to rely on transitive dependencies, or should plugins explicitly require their immediate dependencies?

@joshmoore
Copy link
Member

If I'm following you, then we need to differentiate between internal and public or "API" requirements. Everything I've said is for the former. i.e. if the downstream product happens to also use pyyaml, so be it, but it should then import it (redundantly). In the case of public requirements that are used in method signatures, I agree with you.

@sbesson
Copy link
Member Author

sbesson commented Sep 24, 2020

Man, this was meant to be a minimal change and it looks like I have opened a thread about dependency expectations. Sane albeit painful.

Understood, but assuming there were a bug in 5.8 this would make it difficult for users to rollback, no?

I think so. There is nothing in the plugin that requires the OMERO.py 5.8 API, it is mostly consuming a bug fix which improves the UX. Thinking more about it, I can certainly accept this is not install_requires is about. I propose to close this. Concretly, this means it is the responsibility of the consumer to pip install -U and/or specify the list of pinned requirements.

Since this is one of our own plugins we should follow whatever our "best practice" is, which we need to agree on- is it OK to rely on transitive dependencies, or should plugins explicitly require their immediate dependencies?

Would be useful to look at various guidelines. Looking at https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api for instance, " Software that explicitly depends on the same dependencies as your package should have their own dependency specifications and the author will notice any conflicts. ". In general it seems that semver differentiates the public API contract from the dependency bill/contract.
Happy to use this to define and agree on best practices for ourselves.

@manics
Copy link
Member

manics commented Sep 24, 2020

On the question of the OMERO.py requirement I think it's fine to bump the minimal version to 5.8.0 even if there's no absolute requirement:

  • it makes our maintainance easier (we don't have to test with older versions, not that we do...)
  • there might be a bug in the current version of OMERO.py software doesn't sound like a particularly good reason not to use it, it suggest we haven't tested it properly
  • people can always install the previous version of omero-cli-render

@joshmoore
Copy link
Member

I think it's fine to bump the minimal version to 5.8.0 even if there's no absolute requirement

I agree that in this case it's likely fine, but I don't agree that it's a good general practice, but sounds like Seb wants us to have that conversation somewhere else ;)

@sbesson
Copy link
Member Author

sbesson commented Sep 24, 2020

From https://packaging.python.org/guides/distributing-packages-using-setuptools/#install-requires, “install_requires” should be used to specify what dependencies a project minimally needs to run. I think the minimal qualification is key here.

@sbesson
Copy link
Member Author

sbesson commented Oct 20, 2020

Coming back to this, I can't see a compelling argument to bump the minimal required version of omero-py in setup.py. As for PyYAML, being both a direct and a transitive dependency, explicitly declaring it in setup.py is still probably at least acceptable and we might agree it is good practice.

Closing and we can continue the discussion about managing Python dependencies e.g. in ome/design#103

@sbesson sbesson closed this Oct 20, 2020
@sbesson sbesson deleted the omero-py_5.8.0 branch January 30, 2024 00:07
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.

3 participants