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

Fix connection parameter handling #190

Merged
merged 5 commits into from
Feb 27, 2024

Conversation

pnuu
Copy link
Member

@pnuu pnuu commented Feb 16, 2024

In #165 S3FileSystem() were given all the "extra" kwargs given to the class. For Dispatcher this was only connection_parameters, but for Trollmoves Server this is the full configuration file. The kwarg handling in S3FileSystem and aiobotocore.core.AioSession are such that the values not consumed by the former are passed to the latter. This caused unrecognized kwargs from the Trollmoves Server config being passed to AioSession and as it doesn't have a catch-all **kwargs they cause it to crashed.

This PR changes the config handling in the following way:

  • Dispatcher passes the full config to trollmoves.movers.move_it()
  • S3Mover.copy() gets the dictionary connection_parameters and passes it's items to S3FileSystem()
  • dictionaries can be given in Trollmoves Server .ini configuration

Note that numeric values given in the nested dictionary are not handled.

@pnuu pnuu requested a review from adybbroe February 16, 2024 09:17
@pnuu pnuu self-assigned this Feb 16, 2024
@pnuu pnuu requested a review from mraspaud as a code owner February 16, 2024 09:17
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6e77ed1) 90.98% compared to head (e158f3a) 91.05%.
Report is 2 commits behind head on main.

Files Patch % Lines
trollmoves/server.py 97.14% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
+ Coverage   90.98%   91.05%   +0.06%     
==========================================
  Files          24       24              
  Lines        5247     5298      +51     
==========================================
+ Hits         4774     4824      +50     
- Misses        473      474       +1     
Flag Coverage Δ
unittests 91.05% <98.21%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

S3Mover is unfortunately not the only place where the connection parameters are used, eg the ScpMover also uses this. It is though not tested, so we can't see it from the CI, but these changes very likely break it.
Also, it feels backwards to pass the full config to the movers when only the connection parameters are needed. Instead, I think we need to modify the move_it_server to only pass the connection parameters to the movers.
Or is there a use case I missed when the full config is needed by the movers?

@pnuu
Copy link
Member Author

pnuu commented Feb 16, 2024

S3Mover is unfortunately not the only place where the connection parameters are used, eg the ScpMover also uses this. It is though not tested, so we can't see it from the CI, but these changes very likely break it.

ScpMover does not use connection_parameters dictionary. It was actually only Dispatcher that collected this dictionary and passed it to the movers as extra kwargs whereas Trollmoves Server passes the whole config.

Also, it feels backwards to pass the full config to the movers when only the connection parameters are needed. Instead, I think we need to modify the move_it_server to only pass the connection parameters to the movers. Or is there a use case I missed when the full config is needed by the movers?

In Trollmoves Server we've never had the connection parameters, which is a dictionary, because it uses ini configs. Thus all the items are in the main level of the configuration. Putting these various connection related items in a dictionary would need special item-by-item handling when combining them from the config.

@pnuu
Copy link
Member Author

pnuu commented Feb 16, 2024

Adjusted based on a short discussion on Slack huddle.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Looks good! just a couple of comments.

res["connection_parameters"] = {}
for key in original.keys():
if key in CONNECTION_CONFIG_ITEMS:
res["connection_parameters"][key] = original[key]
Copy link
Member

Choose a reason for hiding this comment

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

should we have a deprecation warning here? to motivate people to move to the __ syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Maybe 🤔 I'll see think about this and see how to document this in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Warning added in e158f3a

@@ -173,14 +173,14 @@ def test_s3_copy_file_to_base_using_connection_parameters(S3FileSystem):
"""Test copying to base of S3 bucket."""
# Get the connection parameters:
config = yaml.safe_load(test_yaml_s3_connection_params)
attrs = config['target-s3-example1']['connection_parameters']
attrs = config['target-s3-example1']
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we be passing only the connection parameters here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right. Forgot to revert this part, done in 53a4bc3

@pnuu pnuu requested a review from mraspaud February 20, 2024 12:37
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM

@mraspaud mraspaud merged commit 22e2dc8 into pytroll:main Feb 27, 2024
6 checks passed
@pnuu pnuu deleted the bugfix-s3filesystem-init branch February 28, 2024 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S3 mover passes all config items to S3FileSystem()
3 participants