-
Notifications
You must be signed in to change notification settings - Fork 5
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 LocalAccessPoint to load EModels from Nexus staged data #111
Allow LocalAccessPoint to load EModels from Nexus staged data #111
Conversation
ab27d6d
to
dda9bf0
Compare
bluepyemodel/access_point/local.py
Outdated
"""Convert the configuration stored in EM_*.json to the format used for final.json.""" | ||
metadata = self.emodel_metadata | ||
return { | ||
metadata.emodel: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can use vars(self.emodel_metadata)
instead of listing all of emodel_metadata
attributes individually here. Also to stay consistent with what is done in store_emodel
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Alternatively, would it make sense to add these keys to the EM_*.json
file, so that all the keys can be retrieved from this file?
It would be a step forward to being able to initialize LocalAccessPoint with just recipes.json
(without manually specifying etype, emodel, iteration)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really...
BluePyEModel has been designed to separate the resource data in the EM_*.json
from the metadata (etype, emodel, iteration, etc.). We expect the user to always give the metadata when creating the access point, which will then fetch the appropriate files form the file system for the local access point or from nexus for the nexus access point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the use case I'm considering, I'm retrieving all the information of the EModel from Nexus and storing them locally.
Then, in a separate step, I want to load the EModel from the local data without querying Nexus anymore.
If the metadata aren't stored in any local file, it won't be possible to load the EModel with just that information, so it seems that the information retrieved from Nexus and stored locally would be incomplete without the metadata.
Alternatively, I can save the metadata from Nexus to an additional file, and I could add for example a function or a class method LocalAccessPoint.from_metadata()
to load this file and initialize the LocalAccessPoint, without modifying the logic of the existing constructor.
This file would act as the entry point for my use case, as it is circuit_config.json for circuits, or simulation_config.json for simulations, or config.json for simulation campaigns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saving the metadata from nexus in an additional file and using it as an entry point sounds fine to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
Is it ok to merge it, or do you have additional changes to do @GianlucaFicarelli ?
Thanks @AurelienJaquier, I'll add some tests with data in the format retrieved from nexus then it's ok to merge |
e257349
to
9b68ea0
Compare
9b68ea0
to
d0496ec
Compare
Change-Id: Id74a0caa20c9cd6d0b21e893034e66123bc5887f
…thers Change-Id: I2c203148cf77465731e28fd9b645906897a047b2
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
+ Coverage 58.15% 59.11% +0.96%
==========================================
Files 103 106 +3
Lines 7380 7512 +132
==========================================
+ Hits 4292 4441 +149
+ Misses 3088 3071 -17 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @GianlucaFicarelli for adding this functionality!
This PR should allow to use LocalAccessPoint to load an EModel from data retrieved from Nexus.
It needs a
recipes.json
file containing the path to the files that have been previously retrieved:This file is similar to the existing
recipes.json
already used by LocalAccessPoint, with these changes:morph_path
: unchangedmorphology
: unchangedparams
: unchanged (the EMC file contains lists of dicts instead of dicts, but it can be opened in the same way)pipeline_settings
: it can be now a string instead of a dict, and in that case the settings are loaded from file.features
contains the path to FCC (seems backward compatible)protocol
removed (it looks like it's present only in the legacy format)final
is a new key that contains the path to the EM file downloaded from Nexus. It will be used to generate on the fly a content compatible withfinal.json
(if the name of the keyfinal
is not clear enough, it could be renamed to something different). If the key is present in recipes.json, thefinal
parameter passed to LocalAccessPoint is used only when saving a newfinal.json
. If the key isn't present, the behaviour is the same as before.There are some files that can be retrieved from Nexus but not present in
recipes.json
. They can be added if needed, and they are:EMS_*.json
(EModelScript)EMW_*.json
(EModelWorkflow)ETC_*.json
(ExtractionTargetsConfiguration)Other changes:
format
andpath
are null in the EMC file from Nexus), or instead of using the defaultmorphology
dir.BluePyEModel/bluepyemodel/access_point/local.py
Line 399 in 15b8dd2
Assuming that the emodel dir contains for example:
then the LocalAccessPoint can be initialized with something like the following:
The parameters
emodel
,etype
,iteration_tag
are still needed because they cannot be determined from recipes.json. We could add them to recipes.json, or to another config file, but it would require other changes to the initialization of LocalAccessPoint.If they are added to
EM_*.json
(that already containsseed
) then they could be automatically determined from this file.