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

feat: fill configs and write to disk on load #438

Conversation

MartinBernstorff
Copy link
Contributor

No description provided.

Copy link
Contributor

github-actions bot commented Nov 15, 2023

Looks like your PR modifies shared library files in psycop/common/.

We highly recommend getting your code reviewed by one of the core maintainers to avoid breaking other projects that depend on these files :-)

@MartinBernstorff
Copy link
Contributor Author

Breaking because of unknown issue with *args, issue here: explosion/confection#47

@KennethEnevoldsen, do you have experience here? ❤️

@KennethEnevoldsen
Copy link
Contributor

I would run the debugger and check what the type actually is.

Btw. the issue on confection is quite hard to follow. I would create a minimal example.

@MartinBernstorff MartinBernstorff linked an issue Nov 15, 2023 that may be closed by this pull request
@MartinBernstorff
Copy link
Contributor Author

MartinBernstorff commented Nov 15, 2023

The type is a dict shaped like:

{'age_filter': {'@preprocessing': 'age_filter', 'min_age': 4, 'age_col_name': 'pred_age'}}}

Looking into writing a minimal reproducible example now.

@MartinBernstorff
Copy link
Contributor Author

Updated the Confection issue with an example :-)

@KennethEnevoldsen
Copy link
Contributor

Hmm odd. You can naturally turn off the type-checking (validate=False). Seem like confection resolved in during filling. However, it seems like it shouldn't be an error as it simply resolve it correctly. Might be that there is a bug in either the config or the resolving.

@MartinBernstorff
Copy link
Contributor Author

Yeah, that succeeds on filling, but removes the * key as well! Have added more context to the issue.

@KennethEnevoldsen
Copy link
Contributor

Added a few additional stuff to the other issues. It seems like it might be a bug.

Copy link
Contributor

This PR is stale because it has been open 1+ days with no activity. Feel free to either 1) remove the stale label or 2) comment. If nothing happens, this will be closed in 7 days.

@MartinBernstorff
Copy link
Contributor Author

I'll close this for now. We can track it in #426.

Copy link
Contributor

This PR is stale because it has been open 1+ days with no activity. Feel free to either 1) remove the stale label or 2) comment. If nothing happens, this will be closed in 7 days.

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.

feat: fill defaults from function signatures into .cfg
2 participants