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

dbs3-client - fix specific version in requirements.txt with == #12102

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

mapellidario
Copy link
Member

@mapellidario mapellidario commented Sep 19, 2024

Related to #12100

Status

not tested.

I am not sure how to build docker images manually without having a new wmcore tag. is it even possible? I tried to read some documentation [1] and skimmed through the scripts in github actions and dockerfiles in dmwm/cmskubernetes/docker/pypi directory but I did not find anything.

Description

Fix the version of the dbs3-client with == operator instead of ~=.

moreover, i did not find any indication of the dbsclient being used in msunmerged and msmonitor. am I wrong?

Is it backward compatible (if not, which system it affects?)

Yes

Related PRs

dmwm/CMSKubernetes#1550
and
dmwm/CMSKubernetes#1551

External dependencies / deployment changes

no change


[1]

@cmsdmwmbot

This comment was marked as outdated.

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand why dbs3-client was installed in the base image.

In general, anyone importing DBS3Reader will have to require dbs3-client, e.g.:

(py3.8) OR-FVFH37J7Q6LR:WMCore amaltar2$ grep -rI DBS3Reader src/python/WMCore/MicroService/*
src/python/WMCore/MicroService/MSOutput/MSOutput.py:from WMCore.Services.DBS.DBS3Reader import getDataTiers

but dbsClient module is imported in this MS module:

(py3.8) OR-FVFH37J7Q6LR:WMCore amaltar2$ grep -rI dbsClient src/python/WMCore/MicroService/*
src/python/WMCore/MicroService/Tools/Common.py:from dbs.apis.dbsClient import aggRuns, aggFileLumis

which means that everyone importing this Common module will have to require dbs3-client.

These 2 dbs3-client functions are actually used in the Common.py module as follows:

def getRunsInBlock(blocks, dbsUrl):
        rows = aggRuns(rows) # adjust to DBS Go server output

def getFileLumisInBlock(blocks, dbsUrl, validFileOnly=1):
        rows = aggFileLumis(rows)  # adjust to DBS Go server output

and it turns out only MSTransferor uses those 2 Common functions.

With that said, I can suggest 2 options:

  1. we make every single microservice to depend on dbs3-client
  2. or, we move the imports inside each function that is actually using it.

Bottom line, AFAICT, the only microservices that are really using dbs3-client are: MSTransferor and MSOutput. The others either don't use it or use directly the REST APIs.

@cmsdmwmbot

This comment was marked as outdated.

@mapellidario
Copy link
Member Author

Thanks Alan for checking! I agree with option (2), install dbs3-client only in microservices that really need it.

PS: I am not sure why the unittest WMCore_t.Services_t.Rucio_t.RucioUtils_t.RucioUtilsTest.testWeightedChoice is failing from this comment :(

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 1 changes in unstable tests
  • Python3 Pylint check: succeeded
    • 5 warnings
    • 55 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 13 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/15223/artifact/artifacts/PullRequestReport.html

@amaltaro
Copy link
Contributor

No worries, that test testWeightedChoice should actually be marked as unstable.
If you want to add it within this PR, this is the file:
https://github.com/dmwm/WMCore/blob/master/test/etc/UnstableTests.txt

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for missing the latest changes, Dario. They are all looking good to me now.

In the future, please make a new review request whenever you are done with your changes, because then we can see it with GitHub filters.

I would ask you to squash these commits together, as documented in https://github.com/dmwm/WMCore/blob/master/CONTRIBUTING.rst#contributing, but I would like to get a new release out today, so we let it go this time.

@amaltaro
Copy link
Contributor

And we have up-to-date base images now with tag pypi-20240923-stable, as linked in the initial description.
Everything should be in place to move forward with this PR. Giving it a try with 2.3.6rc6 soon.

@amaltaro amaltaro merged commit 30110f9 into dmwm:master Sep 23, 2024
2 of 4 checks passed
@mapellidario
Copy link
Member Author

In the future, please make a new review request whenever you are done with your changes, because then we can see it with GitHub filters.
I would ask you to squash these commits together, as documented in https://github.com/dmwm/WMCore/blob/master/CONTRIBUTING.rst#contributing, but I would like to get a new release out today, so we let it go this time.

soorry i did not explicitely ask for a new review! and i did not squash myself so that you could see what was different from the previous review, with the idea that you could click on "squash commits" while merging :) but ok, understood, next time i will squash the commits myself

@amaltaro
Copy link
Contributor

Back in the days, Eric used a tool to watch branches and so on (SourceTree I think, maybe I should give it a try as well). And it did not behave well with the Squash and merge option, so in general we always went with the Merge pull request option.
I don't think there is any blocker to change it though, in case we want it. Thanks Dario!

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