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

Implement support for gzipped stream for RucioConMon #11142

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

vkuznet
Copy link
Contributor

@vkuznet vkuznet commented May 12, 2022

Fixes #11036

Status

ready

Description

Implement support for zipped I/O while calling RucioConMon service. The changes required to introduce binary read/write mode operation on cache and properly read and write files in binary mode.

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

YES

Related PRs

It is similar in nature to #11041 (since it relies on gzip library) but it adjust WMCore service to properly handle binary content in its cache.

External dependencies / deployment changes

None

@vkuznet
Copy link
Contributor Author

vkuznet commented May 12, 2022

@amaltaro , @todor-ivanov it is initial implementation and I'm seeking your feedback. To resolve the issue I fixed Services class which handle all HTTP requests, in particular I added rb and wb mode to reading and writing the content to the cache. So far the cache class only handle non-binary content (like JSON data). The zipped content by definition is binary and should be treated differently. I added integration test too and tested the results in my local environment, and everything seems to be in good shape. I'll await Jenkins tests results to request final review.

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 7 tests added
  • Python3 Pylint check: failed
    • 4 warnings and errors that must be fixed
    • 72 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 12 comments to review

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

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.

@vkuznet I am not sure it would work for every single case, but the idea that came to my mind was to actually define this binary mode in the super class (Service) at the __init__ method. Sub-classes inheriting from it - like RucioConMon - would then define one more option in configDict and pass it over, identifying that its http responses are meant to be dealt with in a binary mode.

I do not know though how to handle a class that we would be fetching human-readable responses and binary at the same time. Perhaps trying to read it as binary first and having a fallback (try/except) to human-text?

Copy link
Contributor

@todor-ivanov todor-ivanov left a comment

Choose a reason for hiding this comment

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

Hi @vkuznet that is great you did provide the implementation of this method.

I had only one minor comment inline. Please take a look. The fix should be trivial.

Thank you.

src/python/WMCore/Services/RucioConMon/RucioConMon.py Outdated Show resolved Hide resolved
@vkuznet
Copy link
Contributor Author

vkuznet commented May 13, 2022

@vkuznet I am not sure it would work for every single case, but the idea that came to my mind was to actually define this binary mode in the super class (Service) at the __init__ method. Sub-classes inheriting from it - like RucioConMon - would then define one more option in configDict and pass it over, identifying that its http responses are meant to be dealt with in a binary mode.

I do not know though how to handle a class that we would be fetching human-readable responses and binary at the same time. Perhaps trying to read it as binary first and having a fallback (try/except) to human-text?

Alan, I'm not sure which use-cases you're referring to, but I preserved the existing ones, and obviously these use-cases are unable to read binary data which I fixed in this PR. Unless you specify all use-cases you envision I don't think we can have any productive discussion on how code should be structured. I also don't think the Service has anything to do with data-format. If you want to have generic behavior you need to introduce intermediate object which will deal with different data-types. For instance, DataIO class which will have methods like open, close, read and write. Such class will be able to treat different data-format according to their specs. Then your Service or anything else can use DataIO class to handle the data for it. This abstraction makes more sense to me since it only encapsulated specific of data IO operations. If you want further abstractions and generalization, then you need to provide CacheIO class which will only deal with cache operations and has generic method as read and write. It should be transparent to the data input/format, i.e. whatever it will be given it will write to cache, etc. And, it will use internally DataIO to perform proper reading and writing to a file.

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 7 tests added
  • Python3 Pylint check: failed
    • 4 warnings and errors that must be fixed
    • 73 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 12 comments to review

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

@amaltaro
Copy link
Contributor

Valentin, what I was proposing is an alternative to the way this binary flag is defined. Instead of passing another flag over to multiple methods, I was considering to define this flag only once (in the __init__ method) and then use it along the code to identify whether data is supposed to be in the binary format or not.

So, the whole logic is basically the same as the one you implemented, the only different is on how you notify the superclass (Services) that you want to deal with binary http requests or not. Given that the Services class is a super class for almost every single package under WMCore/Services, other service wrappers (DBS, CRIC, WMStats, etc) would require to change only one configuration parameter, instead of overriding some methods with the additional binary parameter that you provide here.

I am trying to generalize and make it easier to be adopted in other parts of the code. However, as previously mentioned, it could be that for a given class object we want only part of the calls to be dealt with in a binary mode, and this is what is not clear to me whether we can and how we could solve it.

@vkuznet
Copy link
Contributor Author

vkuznet commented May 13, 2022

Valentin, what I was proposing is an alternative to the way this binary flag is defined. Instead of passing another flag over to multiple methods, I was considering to define this flag only once (in the __init__ method) and then use it along the code to identify whether data is supposed to be in the binary format or not.

So, the whole logic is basically the same as the one you implemented, the only different is on how you notify the superclass (Services) that you want to deal with binary http requests or not. Given that the Services class is a super class for almost every single package under WMCore/Services, other service wrappers (DBS, CRIC, WMStats, etc) would require to change only one configuration parameter, instead of overriding some methods with the additional binary parameter that you provide here.

I am trying to generalize and make it easier to be adopted in other parts of the code. However, as previously mentioned, it could be that for a given class object we want only part of the calls to be dealt with in a binary mode, and this is what is not clear to me whether we can and how we could solve it.

Alan, thanks for clarification, it is impossible to do that in Service class since it does both things, i.e. the data retrieval and cache management. But data retrieval part depends on upstream service, i.e. some services, e.g. DBS/RucioConMon can provide gzipped content, while other, e.g. CRIC, WMStats do not. But since caching part is part of Service and uses open calls the open call is possible for only one mode of operation. Therefore you may have conflict between upstream data content and caching open call. This is why I wrote that these operations should be separated and delegated to concrete classes like DataIO and CacheIO which will abstract common operations like open, read, and write. Until it is done we don't have a choice in Service class (in addition it hanles differnt data-formats, e.g. JSON vs other, which also should be decoupled to make this generalization). But, we can add binary parameter to RucioConMon.__init__ constructor. Please note, that I implemented logic of existing class which already had this parameters in method names, therefore I don't know if it was intentional or not.

I suggest that you make your mind and tell me how you'd like to see it. My intention was only to adjust existing code without major refactoring. As such, I think adding binary to RucioConMOn constructor is a compromise option, and now that I prove that it works, we can make it as default.

@amaltaro
Copy link
Contributor

@vkuznet Valentin, I remember we had a very productive discussion on this topic in the past week, but I no longer remember all the details.

I think we agreed that the best would be to use the http headers (from the response object) as a generic feature. While for RucioConMon, we use the headers but we also keep request format=raw for binary data. Is that correct? Please complement here with anything else that I might have missed. Thanks

@vkuznet
Copy link
Contributor Author

vkuznet commented May 27, 2022

@amaltaro , yes it is correct conclusion.

@vkuznet
Copy link
Contributor Author

vkuznet commented Jun 2, 2022

@amaltaro , let's not forget about this PR, the code is ready and again it is now in the air of awaiting something which I have no control. What is the conclusion how to move forward with this?

@amaltaro
Copy link
Contributor

amaltaro commented Jun 2, 2022

@vkuznet in that case, the conclusion is that we are not going to add another keyword to those methods in the super class Service, and instead it should use the MIME type (or whatever HTTP header we discussed) to know how to read/decode the data from the web service.

@vkuznet
Copy link
Contributor Author

vkuznet commented Jun 3, 2022

@amaltaro I'm more interested what we should do with this PR, it is still unclear from your answer. Please clarify if any action should be taken to move forward with the PR itself.

@amaltaro
Copy link
Contributor

amaltaro commented Jun 10, 2022

@vkuznet Valentin, sorry for not being specific with my previous answer. IMO, we should:

  • remove the binary option from the Service class and instead we should read the HTTP response MIME type to know in which format the response is. When we discussed this a few weeks ago, I remember seeing an application/octet-stream being returned by RucioConMon in such cases.
    • maybe we do not need this binary flag in the RucioConMon module as well and we could simply adopt the HTTP headers design.
  • in cases where the web service is not contacted and we actually access data from the cache, I do not know whether we could use the HTTP request header to read from the cache.
  • for services implementing something slightly different (as format=raw query string in RucioConMon), if needed - and it's worth it - we should simply override the super class module.

If it's still not clear and you prefer to have a chat about this, then we can talk in the beginning of the next week.

@vkuznet
Copy link
Contributor Author

vkuznet commented Jun 10, 2022

Alan, it seems to me that you're mixing different things. The binary option to the superclass is mandatory to write binary content. This content you may get either specifying HTTP header or using HTTP end-point flags (as RucioConMon does). The data arrive in binary format and if you want to write it to the cache you must open file with binary flag for read/write operations. Let's setup chat meeting about it to resolve misunderstandings.

@amaltaro
Copy link
Contributor

amaltaro commented Jul 1, 2022

test this please

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 1 changes in unstable tests
  • Python3 Pylint check: failed
    • 4 warnings and errors that must be fixed
    • 73 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 12 comments to review

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

@amaltaro
Copy link
Contributor

test this please

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 3 tests deleted
    • 4 tests added
    • 1 changes in unstable tests
  • Python3 Pylint check: failed
    • 4 warnings and errors that must be fixed
    • 73 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 12 comments to review

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

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.

@vkuznet I had another look at this implementation and left a few comments for your consideration. Thanks

src/python/WMCore/Services/RucioConMon/RucioConMon.py Outdated Show resolved Hide resolved
src/python/WMCore/Services/RucioConMon/RucioConMon.py Outdated Show resolved Hide resolved
src/python/WMCore/Services/Service.py Show resolved Hide resolved
src/python/WMCore/Services/Service.py Show resolved Hide resolved
src/python/WMCore/Services/Service.py Show resolved Hide resolved
src/python/WMCore/Services/Service.py Outdated Show resolved Hide resolved
@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 3 tests deleted
    • 4 tests added
    • 2 changes in unstable tests
  • Python3 Pylint check: failed
    • 5 warnings and errors that must be fixed
    • 74 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 13 comments to review

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

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 3 tests deleted
    • 4 tests added
    • 2 changes in unstable tests
  • Python3 Pylint check: failed
    • 1 warnings and errors that must be fixed
    • 74 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 13 comments to review

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

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 3 tests deleted
    • 4 tests added
    • 2 changes in unstable tests
  • Python3 Pylint check: succeeded
    • 74 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 13 comments to review

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

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 98 new failures
    • 286 tests deleted
    • 1 tests no longer failing
    • 6 changes in unstable tests
  • Python3 Pylint check: failed
    • 3 warnings and errors that must be fixed
    • 15 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 68 comments to review

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

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 97 new failures
    • 286 tests deleted
    • 1 tests no longer failing
    • 7 changes in unstable tests
  • Python3 Pylint check: failed
    • 3 warnings and errors that must be fixed
    • 15 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 30 comments to review

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

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 1 tests no longer failing
    • 2 changes in unstable tests
  • Python3 Pylint check: succeeded
    • 73 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 13 comments to review

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

@vkuznet
Copy link
Contributor Author

vkuznet commented Jul 20, 2022

Alan, now all test/pylint are clear. Feel free to review again.

@vkuznet vkuznet requested review from amaltaro and todor-ivanov July 20, 2022 14:20
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.

Valentin, it's looking good to me. Since you still have to squash these commits, I left another minor enhancement comment.

Now that you have these final changes, could you please test your integration unit test as well, to make sure that everything is behaving? Thanks

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 1 tests no longer failing
    • 3 changes in unstable tests
  • Python3 Pylint check: succeeded
    • 74 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 13 comments to review

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

Copy link
Contributor

@todor-ivanov todor-ivanov left a comment

Choose a reason for hiding this comment

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

Thanks @vkuznet
This look good to me.

@amaltaro
Copy link
Contributor

@vkuznet once you confirm the unit test is still succeeding, this is ready to go.

@vkuznet
Copy link
Contributor Author

vkuznet commented Jul 20, 2022

Here is results from integration test:

python test/python/WMCore_t/Services_t/Rucio_t/RucioConMon_t.py
.
----------------------------------------------------------------------
Ran 1 test in 2.717s

OK

I hope it is convincing and you can merge this PR :)

@amaltaro
Copy link
Contributor

amaltaro commented Jul 20, 2022

Great! You forgot squashing those commits though (keep in mind to keep test in a separate commit please) and remove those labels, if it's ready to go from your side. @vkuznet

@vkuznet
Copy link
Contributor Author

vkuznet commented Jul 20, 2022

Alan, I squashed all commits, but test file was added as part of initial commit which cannot be separated.

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 1 tests no longer failing
    • 2 changes in unstable tests
  • Python3 Pylint check: succeeded
    • 74 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 13 comments to review

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

@vkuznet
Copy link
Contributor Author

vkuznet commented Jul 20, 2022

Alan, the new failed unit test after the squash is related to CouchDB, see here

CouchPreconditionFailedError - reason: Precondition Failed, data: {} result: b'{"error":"file_exists","reason":"The database could not be created, the file already exists."}\

I have no idea how this is possible, but I really doubt it is related to squash of commits.

@amaltaro
Copy link
Contributor

Thanks for looking into the details of the unit test failure. It's a known issue and meant to be tracked in:
#11194

Regarding unit tests, please put them in separate commits for future PRs.
Given our discussion, I think this PR is ready to go from your side as well, so I went ahead and removed those 2 labels. Thanks

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.

MSUnmerged: Implement Zipped I/O streams
4 participants