Skip to content

safely convert ini parameters to bool, int, list and dict - #104

Open
kgk wants to merge 16 commits into
sontek:masterfrom
kgk:master
Open

safely convert ini parameters to bool, int, list and dict#104
kgk wants to merge 16 commits into
sontek:masterfrom
kgk:master

Conversation

@kgk

@kgk kgk commented Mar 20, 2024

Copy link
Copy Markdown

This will convert ini paramerts to bool or int if they can be.
Allows for max_tasks_per_child to be set as well as others for celery workers

fixes : #103

@kgk kgk mentioned this pull request Mar 25, 2024
Comment thread pyramid_celery/loaders.py
Comment on lines +96 to +97
if value.lower() in ("true", "false"):
return bool(value)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For this, I suggest using pyramid.settings.asbool to consider the full range of supported "bool-like" values.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I did consider this, however, this is trying to replicate what you might use in celeryconfig.py, but with values you would find in the ini file.

For example in celeryconfig.py you might write
task_acks_late = True
so in the ini you would need to write
task_acks_late = true

Pyramid asbool will also convert integer and other values to a boolean which may or may not work similarly to setting them celeryconfig.py

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good point about the integer. Better to stay safe with the explicit bool-like strings. 👍

Comment thread pyramid_celery/loaders.py

for key, value in self.parser.items('celery'):
config_dict[key] = value
config_dict[key] = safe_conversion(value)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might need to consider applying this to other structures below?
For example, a lot of Task result backend settings tend to have very nested dict definitions.

@kgk

kgk commented May 23, 2024

Copy link
Copy Markdown
Author

The latest push adds json5 as safely converted value.. json5 is a human friendly json parser (accepts both ' and " ) and other things. I could also use a python AST parser if you prefer

Comment thread pyramid_celery/loaders.py Outdated
Comment on lines +98 to +103
try:
newval= json5.loads(value)
#print("JSON DECODED", type(newval), newval)
return newval
except JSONDecodeError:
pass

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should probably try using json.load as fallback if pyjson5 is not installed.
The imports should also handle missing pyjson5 from installed packages.
I don't think it is necessary for pyramid_celery to inject this new dependency.

@kgk

kgk commented May 24, 2024

Copy link
Copy Markdown
Author

I feel literal_eval is better solution.

  1. Will only evaluate python literals .. no arbitrary code execution
  2. No new dependencies.

@fmigneault

Copy link
Copy Markdown

I agree, ast.literal_eval is a better approach.

@kgk kgk changed the title safely convert ini parameters to bool and int safely convert ini parameters to bool, int, list and dict May 24, 2024
@kgk

kgk commented Jul 16, 2024

Copy link
Copy Markdown
Author

Just a ping to check if the patch is accepted

Comment thread pyramid_celery/loaders.py Outdated
Comment on lines +99 to +104
try:
if float(value).is_integer():
return int(value)
return float(value)
except ValueError:
pass

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't think this is needed anymore since literal_eval should handle it as well.

@fmigneault

Copy link
Copy Markdown

@kgk
I also didn't get any answer from @sontek with #102. It seems we will never get an official release...
If you want, I can add it to the list of combined improvements as a crim-ca/pyramid_celery@5.0.0b version.

@kgk

kgk commented Jul 20, 2024

Copy link
Copy Markdown
Author

@fmigneault please add to your list

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.

setting worker config results in worker error

3 participants