-
Notifications
You must be signed in to change notification settings - Fork 149
Remove uninstallable revisions from lock files #929
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks @arash77 can you maybe open a PR with the result of such a run. ping @bernt-matthias |
bernt-matthias
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful to have two approaches. Now we can compare. If there is disagreement we need to investigate and otherwise we can be more confident.
Maybe the output (of both approaches) should contain a reasoning why a revision is removed ...
| skipped += 1 | ||
| continue | ||
|
|
||
| all_revs = list(uninstallable) + list(installable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all_revs = list(revisions)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wouldn't include what we miss from the installable list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't uninstallable = set(revisions) - set(installable) implying revisions = uninstallable + installable.
If so, set(all_revs) = uninstallable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if installable has something that wasn’t in the lock file’s revisions?
Then not exactly! set(all_revs) becomes the union of uninstallable and installable, which can include extra entries not present in revisions.
For example:
revisions = ['r1', 'r2', 'r3', 'r4']
installable = ['r5', 'r2']
uninstallable = set(revisions) - set(installable)
all_revs = list(uninstallable) + list(installable)
print(set(all_revs))
# {'r1', 'r2', 'r3', 'r4', 'r5'} → includes 'r5' not in revisions
scripts/fix_outdated.py
Outdated
| ( | ||
| cand | ||
| for cand in installable | ||
| if version_cache.get(cand) == cur_versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm. What if the set of tools in the repo changed? (Not sure if this is covered in my solution :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a tool was added to the tool repo, i.e. for a repo containing more than one tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then should we run this regularly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant was that tools the set of tools in a tool repository might change between revisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't know how to deal with those! Do you have any suggestions?
|
I found out that some tools might not have any revisions to install after this run. Maybe we could add the next revision to replace them, or, if we haven’t already, handle the revisions in a separate workflow. |
How come? Do you have an example? |
|
|
The revision list of all these tools seems non-empty, i.e. there are revisions to install? |
Yes, there are revisions to install, but not present in lock files. |
… and improve logging clarity
|
With the new changes, it replaces the removed ones with a newer revision, even if it does not exist in the lock files. |
|
I'm running my version here https://github.com/bernt-matthias/usegalaxy-eu-tools/actions/runs/18881254213/job/53884716367 since @martenson was also working on this (CESNET/galaxy_tools#21) I'm also pinging him here. |
|
There are some differences between your run and my run. 📄
|
|
I'm just running again. I needed to add |
|
@arash77 we are getting closer. I looked at a few examples and in all cases I would prefer the results of my branch. |
|
The only problems I have with my script are these that I can’t figure out what’s going on!
|
in the update from 6 to 7 the version was not bumped, i.e. 6 became uninstallable
1 = 0.2.20 + 0 so 1, 2, and 4 are uninstallable I do not understand why 3 is removed. Will need more time to look into this, but maybe it helps you? |
0 0.1.0 So 0->3 is expected I do not understand why 5 is removed.
12 1.6.3+galaxy1 I do not understand why 12 is removed. It is also not shown in the dropdown on the top here |
In the API, we don’t see tool versions in all revisions, and the tool version shown there doesn’t seem to match what you found. revision 1 -> 0.2.20+galaxy1 |
I basically tried to get the versions from the diffs reported in the toolshed. Probably I made some mistakes. In the drop down menu in the TS (which I just discovered) 1 and 3 are not shown at all. If I construct the TS URL manually e.g. 3 it shows
The revisions 2 and 4 which can be selected show that the tools are invalid and also In summary this would explain why we go from revision 0 to 5, but I do not get yet, why 1 and 3 are not installable. |
I think yes this is why the data is not right and it seems that it should get fixed from TS (maybe in v2) but for now it seems that it is not possible to find it out with API calls! |
|
I don't think we can fix the old TS and the new TS - who knows. I'm ready to be pragmatic here and if its just a few revisions that don't match implement special handling, or that I remove/hide tools |
|
So which way should we go? With the API or by cloning the repos? |
|
How about using the approach implemented in this PR and storing removed revisions in an extra key of the lock files, eg |
|
Seperate file would be nice |

This could be an alternative to #838