You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by GitBox <gi...@apache.org> on 2021/12/13 14:53:32 UTC
[GitHub] [buildstream] abderrahim opened a new pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
abderrahim opened a new pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546
Also drop roundtrip_load_data that was unused
Fixes #1495
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
abderrahim commented on pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546#issuecomment-1068983455
> Ok so... IIUC we can pretty much merge this and we'll have made some progress, and there are no CI errors because of the new approach this patch takes.
That's correct.
> The other parts which use round_trip_dump() we could have a private utils function which fiddles with a StringIO to cover those parts, which could be fine to do separately.
Yup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
gtristan commented on pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546#issuecomment-993334378
> The problem I'm noticing with this is that if I run the tests in parallel, they often fail with errors like "I/O operation on closed file" or "expected DocumentStartEvent and got MappingStartEvent".
>
> This may be caused by multiple threads trying to use the same object. Do we have something in buildstream that uses thread local storage?
Yes, the `Messenger` uses thread local storage for per-task logging file handles and such.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
gtristan commented on pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546#issuecomment-1066433850
> I've made it so a new instance is created on each call (like it was previously). I can explore thread local storage if you think it's worth it.
I don't see that change here, in master `Messenger` has `_MessengerLocal` which is TLS for the messenger.
Also, can we check if this change fixes `tests/internals/yaml.py::test_roundtrip_dump` which we currently have on XFAIL ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim removed a comment on pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
abderrahim removed a comment on pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546#issuecomment-1067760030
Maybe I wasn't clear enough:
- The implementation in master uses one instance per call (via the current implementation of the deprecated API in ruamel.yaml)
- My first port used a single instance for everything and failed the tests (I assume because the API is not thread safe)
- The latest version of the PR goes back to using one instance per call
- I guess the optimal solution would be to use thread local storage and use one instance per thread (but I didn't implement it).
Note that I only said this is related to #1475, not that it fixes it. It fixes #1495. The remaining deprecation notices are due to usage of ruamel.yaml from outside of `_yaml.pyx`, see https://github.com/apache/buildstream/blob/master/src/buildstream/_frontend/widget.py#L378. The problem with those is that there is no replacement for the APIs that return a string, we would need to use a StringIO or something to pass to ruamel, but if we're doing that anyway maybe it's a good idea to rewrite the code to use a single StringIO for everything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
abderrahim commented on pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546#issuecomment-992580287
The problem I'm noticing with this is that if I run the tests in parallel, they often fail with errors like "I/O operation on closed file" or "expected DocumentStartEvent and got MappingStartEvent".
This may be caused by multiple threads trying to use the same object. Do we have something in buildstream that uses thread local storage?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
gtristan commented on pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546#issuecomment-1067909027
> Maybe I wasn't clear enough:
>
> * The implementation in master uses one instance per call (via the current implementation of the deprecated API in ruamel.yaml)
> * My first port used a single instance for everything and failed the tests (I assume because the API is not thread safe)
> * The latest version of the PR goes back to using one instance per call
> * I guess the optimal solution would be to use thread local storage and use one instance per thread (but I didn't implement it).
>
Ah, you mean one instance _of a YAML handle_ per call... yeah this sounds perfectly good to me.
> Note that I only said this is related to #1475, not that it fixes it. It fixes #1495. The remaining deprecation notices are due to usage of ruamel.yaml from outside of `_yaml.pyx`, see https://github.com/apache/buildstream/blob/master/src/buildstream/_frontend/widget.py#L378. The problem with those is that there is no replacement for the APIs that return a string, we would need to use a StringIO or something to pass to ruamel, but if we're doing that anyway maybe it's a good idea to rewrite the code to use a single StringIO for everything.
Got it.
Ok so... IIUC we can pretty much merge this and we'll have made some progress, and there are no CI errors because of the new approach this patch takes.
The other parts which use `round_trip_dump()` we could have a private `utils` function which fiddles with a `StringIO` to cover those parts, which could be fine to do separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
abderrahim commented on pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546#issuecomment-993879778
I've made it so a new instance is created on each call (like it was previously). I can explore thread local storage if you think it's worth it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
abderrahim commented on pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546#issuecomment-1067758882
Maybe I wasn't clear enough:
- The implementation in master uses one instance per call (via the current implementation of the deprecated API in ruamel.yaml)
- My first port used a single instance for everything and failed the tests (I assume because the API is not thread safe)
- The latest version of the PR goes back to using one instance per call
- I guess the optimal solution would be to use thread local storage and use one instance per thread (but I didn't implement it).
Note that I only said this is related to #1475, not that it fixes it. It fixes #1495. The remaining deprecation notices are due to usage of ruamel.yaml from outside of `_yaml.pyx`, see https://github.com/apache/buildstream/blob/master/src/buildstream/_frontend/widget.py#L378. The problem with those is that there is no replacement for the APIs that return a string, we would need to use a StringIO or something to pass to ruamel, but if we're doing that anyway maybe it's a good idea to rewrite the code to use a single StringIO for everything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
abderrahim commented on pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546#issuecomment-1067760030
Maybe I wasn't clear enough:
- The implementation in master uses one instance per call (via the current implementation of the deprecated API in ruamel.yaml)
- My first port used a single instance for everything and failed the tests (I assume because the API is not thread safe)
- The latest version of the PR goes back to using one instance per call
- I guess the optimal solution would be to use thread local storage and use one instance per thread (but I didn't implement it).
Note that I only said this is related to #1475, not that it fixes it. It fixes #1495. The remaining deprecation notices are due to usage of ruamel.yaml from outside of `_yaml.pyx`, see https://github.com/apache/buildstream/blob/master/src/buildstream/_frontend/widget.py#L378. The problem with those is that there is no replacement for the APIs that return a string, we would need to use a StringIO or something to pass to ruamel, but if we're doing that anyway maybe it's a good idea to rewrite the code to use a single StringIO for everything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
gtristan commented on pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546#issuecomment-1066466067
> The problem I'm noticing with this is that if I run the tests in parallel, they often fail with errors like "I/O operation on closed file" or "expected DocumentStartEvent and got MappingStartEvent".
>
> This may be caused by multiple threads trying to use the same object. Do we have something in buildstream that uses thread local storage?
I ran the full tests with `-n 12` and didn't see any of these errors... I'll run it again and report back if I see any of these...
Also, after running the full tests, I'm still seeing:
```
/home/tristan/work/buildstream/.tox/py39-nocover/lib/python3.9/site-packages/buildstream/element.py:1684: PendingDeprecationWarning:
round_trip_dump will be removed, use
yaml=YAML()
yaml.dump(...)
instead
env_dump = yaml.round_trip_dump(self.get_environment(), default_flow_style=False, allow_unicode=True)
```
Although this looks like much less deprecation warnings than before the patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim merged pull request #1546: _yaml.pyx: port roundtripping code to the new ruamel.yaml API
Posted by GitBox <gi...@apache.org>.
abderrahim merged pull request #1546:
URL: https://github.com/apache/buildstream/pull/1546
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org