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

Initial Folder structure #161

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Initial Folder structure #161

wants to merge 12 commits into from

Conversation

caviri
Copy link
Contributor

@caviri caviri commented Jun 24, 2024

Let's follow a folder structure that can be based on the organization of MongoDB. This goes as follows:

components 
users
   - digital_twins
     - executions
       - steps
         - odtp-input
         - odtp-output

In this PR I implemented the setup of those two main folders. The project folder would be similar to the digital_twins folder. The rest of the folder structure remains as usual.

@caviri caviri requested a review from sabinem June 24, 2024 14:44
Copy link
Contributor

@sabinem sabinem left a comment

Choose a reason for hiding this comment

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

@caviri Do we really need this now? The approach to setup a folder structure like this does not convince me:

components 
users
   - digital_twins
     - executions
       - steps
         - odtp-input
         - odtp-output

In reality what we need when we work would rather be

components
digital_twins

Then under digital_twins, you would have your named executions, when you prepare and run them.
What might be helpful might be to derive the execution directory name by slugifying digitaltwin name and execution title together.

@@ -17,6 +18,8 @@ def initiate():
odtpS3.create_folders(["odtp"])
odtpS3.close()

create_folders()
Copy link
Contributor

Choose a reason for hiding this comment

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

I struggle to understand this PR: you add two calls create_folder and delete_folder, but then the functions you add are create_main_folders and delete_main_folders.

suggestion: I would rename these function to make it clearer that they belong to the filesystem setup. But then also what are the implications of these folders: if they are there the working directory in the dashboard should also be adjusted, but a path user is not very helpful in that regard: maybe there it would be better to have an ODTP_PATH/github-username home directory that is set for the user.

I am not yet convinced about this PR. I don't think it is necessary for the docker-compose case and for the server setup I would leave this to the server administrator.

@caviri
Copy link
Contributor Author

caviri commented Jun 26, 2024

Hello @sabinem, the problem with that approach is that multiple users (and even the same user) can have overlapping names. I think the user folder would keep things separated and

The multiuser environment (even without authorizarion) is something that's gonna happen when CSFM will deploy this on a server and used via SSH.

On the other hand the naming for the DT should be unique. I'm proposing to use the mongoID on the name, unless we enforce the use of unique names on the interfaces. This is problematic if the name is required to be edited afterwards. The ID never changes but the name yes.

If we accept the ID for the folder name then I see no problem on having one single folder for all DTs.


def create_main_folders(odtp_path = config.ODTP_PATH):
components_folder = os.path.join(odtp_path, 'components')
users_folder = os.path.join(odtp_path, 'users')
Copy link
Contributor

Choose a reason for hiding this comment

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

@caviri a general users folder does not help to separate users: for that there needs to be a folder per user by the github username for example such as

caviri/
sabinem/
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @sabinem, sorry I didn't clarify but the users folders contains one folder per user, similar to digital_twins containing one folder per DT and so.

components/
components/componentA
....
users/
users/sabinem/
users/caviri/
....
users/sabinem/digital_twins/
users/sabinem/digital_twins/digital_twinA
users/sabinem/digital_twins/digital_twinB
...
users/sabinem/digital_twins/digital_twinA/executions/
users/sabinem/digital_twins/digital_twinA/executions/executionsA

The reason why not to put directly digital_twinA or digital_twinA under users is to provide some compartmentalization in case there's another object that the user can have in a future. This allows expanding without the need of changing the path for DTs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@caviri Okay I understand, but then we would also have to adapt the dashboard to always be on the correct working directory. I am not sure what implication that would have on the dashboard. The change seems a bit last minute to me. If you really think it is needed then go ahead with it.

The PR is not finished yet, correct?

@caviri caviri force-pushed the initial-folder-structure branch 3 times, most recently from ef278f1 to 7648547 Compare June 28, 2024 08:42
@@ -41,6 +44,23 @@ def prepare(
document_id=execution_id,
collection=db.collection_executions
)

if not project_path:
#Inferring project path from naming convention
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please put this also into a helper: how to get the project-path for an execution from the execution_id: that way I could also consider to use it on the gui.

That function can then also be reused by execution run

@@ -78,6 +98,23 @@ def run(
document_id=execution_id,
collection=db.collection_executions
)

if not project_path:
#Inferring project path from naming convention
Copy link
Contributor

Choose a reason for hiding this comment

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

See above: make a reusable helper function for this and call it here.

odtp/cli/new.py Outdated
@@ -24,6 +29,19 @@ def user_entry(
):
"""Add new user in the MongoDB"""
user_id = db.add_user(name=name, github=github, email=email)

user_doc = db.get_document_by_id(
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this into the helper function generate_user_path

odtp/cli/new.py Outdated
dt_id = db.add_digital_twin(userRef=user_id, name=name)

dt_doc = db.get_document_by_id(
Copy link
Contributor

Choose a reason for hiding this comment

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

These db calls belong to generate_digital_twin_path: add them there: these functions should be black boxes, that know themselves what to do.

odtp/cli/new.py Outdated
@@ -116,6 +154,29 @@ def execution_entry(
if component_tags:
component_versions = ",".join(odtp_parse.parse_component_tags(component_tags))

execution_doc = db.get_document_by_id(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same put the db calls into the helper function

sabinem and others added 6 commits July 2, 2024 11:20
project forlder name is derived from the execution title
on folder create there is always a new folder created in the
users workspace so that the user will be able to easily run the
selected execution
…w item and folder retrieval on execution preparation/run
@sabinem sabinem changed the title Initial Folder structure Release > 0.4.0: Initial Folder structure Jul 2, 2024
@sabinem sabinem changed the title Release > 0.4.0: Initial Folder structure Initial Folder structure Jul 2, 2024
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