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

allow programmatic overridding of yaml config #2105

Closed
cristianmtr opened this issue Mar 2, 2021 · 10 comments
Closed

allow programmatic overridding of yaml config #2105

cristianmtr opened this issue Mar 2, 2021 · 10 comments
Assignees
Labels
area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing priority/backlog Agreement that this is a nice-to-have, but no one's can work on it now. Community support welcome type/feature-request This issue describes a new feature or behaviour a user or users desires.

Comments

@cristianmtr
Copy link
Contributor

Atm we have some tests in core that create a workspace in hardcoded paths (based on resources), like

def test_crud_in_readme(mocker):

This leads to tests failing and time wasted trying to debug it until you realize that it was reusing the workspace from an older local run. In CI we don't have this problem. We should not have tests that do this, I propose.

The problem is that we are using this _index.yml from resources which has a hardcoded path. Is there any way to override the metas of the indexer programatically in Python?

@cristianmtr cristianmtr self-assigned this Mar 2, 2021
@cristianmtr cristianmtr added area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing type/feature-request This issue describes a new feature or behaviour a user or users desires. type/maintenance This issue is not a bug or a feature_request. and removed type/maintenance This issue is not a bug or a feature_request. labels Mar 2, 2021
@FionnD
Copy link
Contributor

FionnD commented Mar 3, 2021

Following the refinement meeting today, there needs to be a design discussion for how we solve this ticket.

Specifically focusing on how we solve the issue _index issue. This needs some creative thinking on how we add the workspaces.

I have set up a 30-minute meeting to discuss solutions. To make the meeting as quick as possible, if you have thoughts beforehand please post them here ⬇️

@FionnD FionnD added the priority/important-soon Must worked on either currently or very soon. Ideally next release. label Mar 3, 2021
@cristianmtr
Copy link
Contributor Author

cristianmtr commented Mar 3, 2021

Findings:

Passing kwargs directly to .add doesn't work:

    with Flow().add(uses='_index',
                    **{'name': 'ws_from_code'}
                    ) as f:
        f.index(docs, on_done=mock)

In .add the name is picked up properly. Below pod.name is "ws_from_code".

        pod = BasePod(args, needs=needs)
        op_flow._pod_nodes[pod_name] = pod

Also, the name is kept also in the BasePea and ZEDRuntime classes.

The problem seems to be that if you provide a uses= it doesn't maintain this name. The name is only for the ZEDRuntime and BasePea, not for the ._executor in the runtime.

See here:

self._executor = BaseExecutor.load_config(self.args.uses,

@FionnD FionnD added priority/backlog Agreement that this is a nice-to-have, but no one's can work on it now. Community support welcome and removed priority/important-soon Must worked on either currently or very soon. Ideally next release. labels Mar 12, 2021
@cristianmtr
Copy link
Contributor Author

#2162 partially fixes this for one specific test. We can use this approach on a per-test basis.

However, we still want in the future to be able to override / specify context args / env. vars. from Python to override any YAML config. Should keep this issue open

@cristianmtr cristianmtr changed the title remove hardcoded paths in tests allow programmatic overridding of yaml config Mar 23, 2021
@JoanFM
Copy link
Member

JoanFM commented Apr 26, 2021

Closing this issue, as landscape for executors and configuration will heavily change

@JoanFM JoanFM closed this as completed Apr 26, 2021
@cristianmtr
Copy link
Contributor Author

Closing this issue, as landscape for executors and configuration will heavily change

Will that address this issue?

@JoanFM
Copy link
Member

JoanFM commented Apr 27, 2021

Closing this issue, as landscape for executors and configuration will heavily change

Will that address this issue?

I would reopen later given the new semantics, since quite likely all these observations will be outdated

@cristianmtr
Copy link
Contributor Author

Closing this issue, as landscape for executors and configuration will heavily change

Will that address this issue?

I would reopen later given the new semantics, since quite likely all these observations will be outdated

But the underlying point is still valid

@JoanFM
Copy link
Member

JoanFM commented Apr 27, 2021

ok let's reopen then

@JoanFM JoanFM reopened this Apr 27, 2021
@cristianmtr
Copy link
Contributor Author

@JoanFM we can do this now with override_with and override_metas, right?

@JoanFM
Copy link
Member

JoanFM commented Jul 12, 2021

yes @cristianmtr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing priority/backlog Agreement that this is a nice-to-have, but no one's can work on it now. Community support welcome type/feature-request This issue describes a new feature or behaviour a user or users desires.
Projects
None yet
Development

No branches or pull requests

3 participants