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

Context structuring #2432

Merged
merged 7 commits into from
Apr 12, 2023
Merged

Context structuring #2432

merged 7 commits into from
Apr 12, 2023

Conversation

Hind-M
Copy link
Member

@Hind-M Hind-M commented Mar 31, 2023

This is a first pass on Context structuring.
I'm holding back any additional changes on this PR since it already involves many files (even if the changes are quite straightforward)...
If this is the way to go, work can be resumed on the Context entirely in another PR.

@Hind-M Hind-M marked this pull request as ready for review April 6, 2023 09:43
@Hind-M Hind-M requested a review from JohanMabille April 6, 2023 13:18
@Hind-M Hind-M mentioned this pull request Apr 11, 2023
@Hind-M Hind-M self-assigned this Apr 11, 2023
@Hind-M Hind-M changed the title [WIP] Context structuring Context structuring Apr 12, 2023
@Klaim
Copy link
Member

Klaim commented Apr 12, 2023

While I generally approve, I feel like the data being grouped are not "info" but "config(uration)". It's the configuration driving the library/program, it's not just various information or status. If I am correct, consider renaming? Hopefully it's not too hard to do using refactoring tools.

@Hind-M
Copy link
Member Author

Hind-M commented Apr 12, 2023

While I generally approve, I feel like the data being grouped are not "info" but "config(uration)". It's the configuration driving the library/program, it's not just various information or status. If I am correct, consider renaming? Hopefully it's not too hard to do using refactoring tools.

Yeah there is a lot of it which is configuration, I used Info because I was planning to separate configurable and internal/non configurable parameters but at the end we decided to leave it like this (mixing both in the same struct and grouping only by purpose/use instead). If we rename Info to Config I am afraid that it could be confusing since everything is not configurable... But at the same time, using Info could be not that relevant as you pointed out. So there is a choice to make here...

@JohanMabille
Copy link
Member

JohanMabille commented Apr 12, 2023

=> Params: can be from configuration or from the CLI or from anywhere, and it's more flexible regarding what we can move out of / to Configuration etc.

@Klaim
Copy link
Member

Klaim commented Apr 12, 2023

Hmmm well maybe it's not too important and keeping it vague might be better for now... I'll let you decide.

Now just "design" seems not clear to me, so maybe that needs to change - otherwise I'm approving this PR 👍🏽

@JohanMabille JohanMabille merged commit 40c220c into mamba-org:main Apr 12, 2023
20 checks passed
@Hind-M Hind-M deleted the ctx_refactor branch April 13, 2023 06:51
costrouc added a commit to conda/conda-libmamba-solver that referenced this pull request Apr 24, 2023
The following commit to mamba mamba-org/mamba#2432 changes the way in which the context object is constructed. This means the next libmambapy version will break conda-libmamba-solver. Will be opening a PR on the fixes but the bigger question is how to preserve compatibility with older libmamba versions.

We are going to have to continue with repodata patches potentially due to this? Hope this issue/PR opens a larger discussion.

```python
ctx = libmambapy.context.Context()
ctx.json # Attribute Error
ctx.output_config.json # exists
```

This PR should not be merged since mamba with these changes has not
been released yet.
jaimergp added a commit to conda/conda-libmamba-solver that referenced this pull request Jul 17, 2023
* Upcoming changes in mamba context breaks conda-libmamba-solver

The following commit to mamba mamba-org/mamba#2432 changes the way in which the context object is constructed. This means the next libmambapy version will break conda-libmamba-solver. Will be opening a PR on the fixes but the bigger question is how to preserve compatibility with older libmamba versions.

We are going to have to continue with repodata patches potentially due to this? Hope this issue/PR opens a larger discussion.

```python
ctx = libmambapy.context.Context()
ctx.json # Attribute Error
ctx.output_config.json # exists
```

This PR should not be merged since mamba with these changes has not
been released yet.

* add getattr safeguards ans group things together

* pre-commit

* add news [skip ci]

---------

Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
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.

None yet

3 participants