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

Expanding the python path #63

Merged
merged 7 commits into from
Nov 30, 2023
Merged

Expanding the python path #63

merged 7 commits into from
Nov 30, 2023

Conversation

liamhuber
Copy link
Member

To use pympipool in other repos e.g. `pyiron_workflow, we either need to invoke unit tests from the directory the test files live in, or expand the python path to include those files (cf. this issue and this PR with examples). Unfortunately, github does not make it easy to pass env variables to reusable workflows. So, instead, here I'm introducing a new action for easily adding local directories (i.e. prepending the full runner path to the local repo paths provided) to the python path. It needs to be invoked every job since the env is not shared between jobs, so I've started doing that in the reusable workflows based on a boolean flag input.

I still haven't found a great way to test and debug these workflows, so for the moment the reusable workflow actually has an explicit reference to this branch's tag.

@liamhuber
Copy link
Member Author

When the targets here were set to be self-referential for the branch, it worked like a charm over on pyiron_workflow.

Useage defaults to false, so there's no change for other repos leaning on the centralized CI.

@liamhuber
Copy link
Member Author

Ack, I forgot a thing. Will do it tomorrow.

TODO: add a section on the new action to the readme.

@liamhuber
Copy link
Member Author

@pmrv I see you're active now...could you take a couple minutes and review this for me? It's holding up pyiron_workflow #77 so I'm in a rush, but I would really rather not merge to this repo totally without review.

Copy link
Contributor

@pmrv pmrv left a comment

Choose a reason for hiding this comment

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

I don't follow the workflow details, but if you say you have tested it, it seems an okay work around short term.

I do think messing with sys.path in pympipool is an absolutely terrible idea, that is bound to break other things sooner or later so we should discuss this in a meeting if there's not another way to do it.

@liamhuber
Copy link
Member Author

I don't follow the workflow details, but if you say you have tested it, it seems an okay work around short term.

Good enough for me. It all defaults False anyway so you should only see it if you ask for it.

@jan-janssen and @niklassiemer, you are still extremely welcome to provide post-facto review and I'm happy to patch this accordingly.

@liamhuber
Copy link
Member Author

I do think messing with sys.path in pympipool is an absolutely terrible idea, that is bound to break other things sooner or later so we should discuss this in a meeting if there's not another way to do it.

@pmrv, I don't think anyone's going to disagree with you there. Neither Jan nor I see an alternative atm (although my lack of insight should not be taken too seriously as I'm definitely still not an expert on the async stuff), but I'll add it to the agenda for Monday and we can discuss it in more detail.

@liamhuber liamhuber merged commit b5b6bdf into main Nov 30, 2023
@liamhuber liamhuber deleted the tests_in_pythonpath branch November 30, 2023 21:22
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.

2 participants