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

Fix testing #6

Merged
merged 3 commits into from
Feb 13, 2024
Merged

Conversation

ndaelman-hu
Copy link
Collaborator

Minor changes to enable testing (when run in the dev setup from the NOMAD project folder dependencies/schema/simulation/data).

@ndaelman-hu ndaelman-hu added bug Something isn't working documentation Improvements or additions to documentation labels Feb 9, 2024
@ndaelman-hu ndaelman-hu self-assigned this Feb 9, 2024
@JosePizarro3
Copy link
Collaborator

JosePizarro3 commented Feb 9, 2024

What is the issue here? I think the relative imports, especially in __init__.py were correct. See simulation/run e.g.

@ladinesa, any idea of this?

@ndaelman-hu
Copy link
Collaborator Author

What is the issue here? I think the relative imports, especially in __init__.py were correct. See simulation/run e.g.

@ladinesa, any idea of this?

They weren't working for me. Even after fixing this, there's still a problem with matid.classifications. Am looking into it now.

Getting relative imports to work is a bit tricky, especially with nested projects. If you cross-reference with other dependencies, you'll see that they also follow this kind of import statement, i.e. relative to their own project root. That is like an absolute path from their perspective and thus robuster.

@ladinesa
Copy link
Collaborator

ladinesa commented Feb 9, 2024

What is the issue here? I think the relative imports, especially in __init__.py were correct. See simulation/run e.g.

@ladinesa, any idea of this?

sorry I cannot follow the problem. In your simulationdata/init.py you have from .system import ModelSystem, so in principle your can do from simulationdata import ModelSystem, from simulationdata.system import Model won't work unles you have from . import system

@ndaelman-hu
Copy link
Collaborator Author

@JosePizarro3 Fixed the matid import, but hit another dead end. I either use your branch 1728, but it hasn't been rebased to today's changes yet.

Moreover, I also suggest that for the time being we keep the classes that you introduce there in this plugin (for now, while we're generating a structure), unless it really breaks something.

@JosePizarro3
Copy link
Collaborator

These relative imports should and do work. The imports in MatID that you implemented are anyways wrong.

In any case, please, wait for #1 to be merged. Now you are working on top of develop.

I will push the changes to gitlab next week.

@ndaelman-hu
Copy link
Collaborator Author

Adding __init__.py to tests and re-building has solved the issue, while maintaining the relative paths.
Kudos goes to @ladinesa . He also pointed out that for dynamically installed Python envs, absolute paths are fine (better even), but not when dealing regular pip installs.

@JosePizarro3 I rebased to 1-initial-polishing-of-modelsystem. The effective change is just adding an empty file, so you can also apply this directly to your branch before merging.

@JosePizarro3
Copy link
Collaborator

Adding __init__.py to tests and re-building has solved the issue, while maintaining the relative paths. Kudos goes to @ladinesa . He also pointed out that for dynamically installed Python envs, absolute paths are fine (better even), but not when dealing regular pip installs.

@JosePizarro3 I rebased to 1-initial-polishing-of-modelsystem. The effective change is just adding an empty file, so you can also apply this directly to your branch before merging.

Can you target the branch 1-initial-polishing? :)

@JosePizarro3 JosePizarro3 changed the base branch from develop to 1-initial-polishing-of-modelsystem February 13, 2024 08:41
@JosePizarro3 JosePizarro3 merged commit b80964e into 1-initial-polishing-of-modelsystem Feb 13, 2024
2 checks passed
@JosePizarro3 JosePizarro3 deleted the fix_testing branch February 13, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants