Fix/requirements second version

[drodarie]

Describe the work done

• Change ConfigurationDictAttribute and ConfigurationListAttribute fill function to expose their children __inv__ function.
• Change PackageRequirement _inv__ function
• Extend and document CodeDependencyNode usages. Now it fetches a python file or a module based on provided string.
• Fix README links
• Fix test_placement

List which issues this resolves:

close #842

Tasks

• Added tests for CodeDependencyNode and PackageRequirement
• Updated documentation of CodeDependencyNode

Note

@Helveg mentioned in #843 that tree_of should not have an elif as it could happen that the result of __tree__ is not yet fully parsed but then this fails the test: test_configuration.test_code_dependency_node. I would need some feedback on the current implementation

[drodarie]

Realized this version does not work neither 😭
My solution in the fill function of ConfigurationDictAttribute and ConfigurationListAttribute expose __inv__ as a static attribute of the class not of its instance.
Back to draft board.

[Helveg]

Realized this version does not work neither 😭 My solution in the fill function of ConfigurationDictAttribute and ConfigurationListAttribute expose __inv__ as a static attribute of the class not of its instance. Back to draft board.

Yea, I mentioned this in my review:

you can not set this on the class, ypu must set this on an instance, otherwise you are setting a global shared inv for all conf list attributes

Could you resolve all conversations of my previous review?

[Helveg]

Oooooh, oops, I hadn't posted this review yet :)

[Helveg]

just if here

[Helveg]

unless you have some sort of use case and can show with tests that it js backwards compatible and a test for the new use case that breaks without this elif

[drodarie]

Yes as I explained in the PR description, test_code_dependency_node does not work without the elif.
Now this new test is checking my changes in the code. So maybe most probably, I did something wrong 😅

[drodarie]

Ok I have added a test in inv for CodeDependencyNode. I now do not need the elif anymore.

[Helveg]

what is this supposed to achieve?

[Helveg]

The module_file should ALWAYS be exactly the file the user orders it to be in the configuration, AND it MUST be relative to the project root directory, otherwise the file dependency system's behaviour is undefined and might do weird things.

[drodarie]

Ok I want to give two possibilities to the user:
Either they should provide a real string path to the python file. e.g: /path/to/bsb-test/configs/double_neuron.py
Otherwise you provide a import like module string. e.g: bsb-test.configs.double_neuron

If you are in the first case (it match this condition), you get rid of the extension and replace the "/" with "." (like it was done before)
If you are in the second case, you find the file attached to the module.

[Helveg]

But this leaves the possibility that the object in sys.modules doesn't have a __file__ attribute at all, and that the model description depends on the state of the Python interpreter, i.e., has someone already imported something in the currently running Python process? The crux of code dependencies is that they need to be files relative to the project root anyways, so even when they use a.b.c it should resolve to ./a/b/c.py anyway.

I am fine with allowing both formats (and then promoting 1 of the 2 in the docs so we can deprecate and switch to the most useful one over time), but it would have to be a matter of string parsing, I would not involve sys.modules

[drodarie]

Ok I thought that a sys.modules object would always have a __file__ attribute. It is also a good point that importing might cause some additional issue. So, let's drop the sys.modules parsing.
Regarding the a.b.c format, I am not really convince since if one of the folder contains a "." then, the conversion back to .a/b/c.py would results in a bug.
For instance, if b = "Python3.10" then a.b.c results in ./a/Python3/10/c.py

[drodarie]

Ok you convinced me.

[Helveg]

The issue here is mostly that without documentation this whole system is going to be very unclear to users. So if you have the time at all, write a guide on how to properly use packages and files in the BSB

[drodarie]

I fully agree ! Unfortunately, I do not have the time to do this needed documentation and I am urged to publish a new version of bsb next week. I promise to come provide more documentation on this matter with the next release.

By the way, do you know why the API test is failing here? I cannot replicate it on my PC

[Helveg]

I fixed that once before already 🤔 look closely:

- BoxTree: typing.Type["bsb.voxels.BoxTree"]
?                           ^^^ ^

+ BoxTree: typing.Type["bsb.trees.BoxTree"]

It appears both bsb.voxels and bsb.trees export a public API member called BoxTree, only bsb.trees should provide it. The API check script should probably also detect these collisions.

[drodarie]

Found it! It was on the geometric shapes branch

[Helveg]

The module_file should ALWAYS be exactly the file the user orders it to be in the configuration, AND it MUST be relative to the project root directory, otherwise the file dependency system's behaviour is undefined and might do weird things.