[23.0] Fix parsing tool metadata from bio.tools

[kysrpex]

Galaxy issue #16445. Related, closed PRs: #16447, #16448, #16450.

---

Parsing bio.tools data for tool metadata is broken, probably because of research-software-ecosystem/content#fd9ed50 or research-software-ecosystem/content#7b40d92 (sorry, we are using this feature on usegalaxy.eu just since last night). The keys "function" and "topic" do not exist any longer for many tools. Thus, such tools fail to load.

• Sample Sentry error report from usegalaxy.eu (key "function" missing):
  https://sentry.galaxyproject.org/organizations/galaxy/issues/123152/events/291df96a5e694baa978051863bb16e51/

[...]
Error reading tool from path: toolshed.g2.bx.psu.edu/repos/devteam/get_flanks/077f404ae1bb/get_flanks/get_flanks.xml
[...]
exception KeyError: 'function'

• Sample Sentry error report from usegalaxy.eu (key "topic" missing): https://sentry.galaxyproject.org/organizations/galaxy/issues/123151/events/9156051efa734ae09f7643d86fa54ac6/

[...]
Error reading tool from path: toolshed.g2.bx.psu.edu/repos/iuc/b2btools_single_sequence/b694a77ca1e8/b2btools_single_sequence/b2btools_single_sequence.xml
[...]
exception KeyError: 'topic'

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

We know this works from fixing it in production on usegalaxy.eu after noticing that many of our tools disappeared, but proper test coverage would be needed. I am leaving the test coverage to you.

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[kysrpex]

Closing due to this comment.

[nsoranzo]

23.0 is probably the right one, we don't normally support releases older than a year, see https://github.com/galaxyproject/galaxy/blob/dev/SECURITY.md#supported-versions .

[kysrpex]

23.0 is probably the right one, we don't normally support releases older than a year, see https://github.com/galaxyproject/galaxy/blob/dev/SECURITY.md#supported-versions .

Sorry, then I will reopen again 😅.

[wm75]

Just a stylistic issue, but shouldn't from_json be a classmethod rather than a staticmethod?

[kysrpex]

Just a stylistic issue, but shouldn't from_json be a classmethod rather than a staticmethod?

That's true, it would make better sense for it to be a class method (devs: feel free to commit changes).

[nsoranzo]

Just a stylistic issue, but shouldn't from_json be a classmethod rather than a staticmethod?

That's true, it would make better sense for it to be a class method (devs: feel free to commit changes).

No, classmethods have the special cls as first parameter, which is not needed here, so staticmethod is correct.

[nsoranzo]

@kysrpex Can you have a look at this comment #16447 (comment) please?

[kysrpex]

Just a stylistic issue, but shouldn't from_json be a classmethod rather than a staticmethod?

That's true, it would make better sense for it to be a class method (devs: feel free to commit changes).

No, classmethods have the special cls as first parameter, which is not needed here, so staticmethod is correct.

Why would a class method be incorrect, if referencing the class itself is precisely what line 22 is doing? Class BiotoolsEntry is referencing class BiotoolsEntry.

[kysrpex]

@mvdbeek commented:

Is there some background on why those terms were removed ? Is there something to be reported upstream about this breaking change ?

At the moment I am just as aware as you are. We enabled the feature and it did not work due to those keys being missing. If I learn why the keys were removed then I let you know.

[bgruening]

bio.tools entries do not NEED to contain EDAM terms. This is known, not good, but we need to live with this or annotate them.

[mvdbeek]

and are they actively being removed, or do you see the errors for tools that didn't have any biotools entry previously ?

[kysrpex]

and are they actively being removed, or do you see the errors for tools that didn't have any biotools entry previously ?

The thing is that we had this feature disabled, right @bgruening? So we did not have the chance to see errors until now. Please correct me if I am wrong because I am not 100% sure of what I am saying.

[bgruening]

I try to summarize what I have seen so far.

• we enabled the github clone over the weekend together with our AU friends
• tools that previously did not have any EDAM annotation have now annotations
• on Monday during a workshop people fund that tools have been missing
• those missing tools have not been loded because of the lookup to the local bio.tools/content checkout failed
• it failed because the json files on the harddrive inside the local checkout of the bio.tools/content repo DID exist, but had missing meta-data

[nsoranzo]

Just a stylistic issue, but shouldn't from_json be a classmethod rather than a staticmethod?

That's true, it would make better sense for it to be a class method (devs: feel free to commit changes).

No, classmethods have the special cls as first parameter, which is not needed here, so staticmethod is correct.

Why would a class method be incorrect, if referencing the class itself is precisely what line 22 is doing? Class BiotoolsEntry is referencing class BiotoolsEntry.

It's not about referencing the class, it's about not having cls as first parameter, see e.g. https://www.geeksforgeeks.org/class-method-vs-static-method-python/

[wm75]

@nsoranzo not that it makes a big difference here, but my point was precisely that the current staticmethod returns an instance of a hard-coded type (BiotoolsEntry), when it could use the cls argument available in a classmethod, just like it's done in many places of the stdlib (e.g. https://github.com/python/cpython/blob/8725d36fda2125fd089e6a714ba70089ba7498db/Lib/collections/__init__.py#L295) to implement "alternative constructors".

[kysrpex]

@nsoranzo In the link that you have provided, you can find this example:

# Python program to demonstrate
# use of class method and static method.
from datetime import date


class Person:
	def __init__(self, name, age):
		self.name = name
		self.age = age

	# a class method to create a Person object by birth year.
	@classmethod
	def fromBirthYear(cls, name, year):
		return cls(name, date.today().year - year)

	# a static method to check if a Person is adult or not.
	@staticmethod
	def isAdult(age):
		return age > 18


person1 = Person('mayank', 21)
person2 = Person.fromBirthYear('mayank', 1996)

print(person1.age)
print(person2.age)

# print the result
print(Person.isAdult(22))

I see the method fromBirthYear analogous to the method from_json.

[kysrpex]

@nsoranzo not that it makes a big difference here, but my point was precisely that the current staticmethod returns an instance of a hard-coded type (BiotoolsEntry), when it could use the cls argument available in a classmethod, just like it's done in many places of the stdlib (e.g. https://github.com/python/cpython/blob/8725d36fda2125fd089e6a714ba70089ba7498db/Lib/collections/__init__.py#L295) to implement "alternative constructors".

The example from the standard library is also pretty good.

[nsoranzo]

@wm75 @kysrpex Ah, yes, that would make sense, I didn't understand you wanted to change the content of the method in addition to its decorator.

[mvdbeek]

@github-actions github-actions bot added this to the 23.0 milestone 2 days ago

that's not right, at least for the current setup where we write release notes once ... do we need to adjust the target milestone on older branches as well? PRs against dev are milestoned correctly

[mvdbeek]

Let's keep the refactoring for dev. Thanks for the fix!