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

Proposed code changes for Remote Processing #76

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

linuxguy123
Copy link

No description provided.

Initial add for CfdOF remote processing
Uploaded to wrong dir
Uploaded to wrong dir
Uploaded to wrong dir
Uploaded to wrong dir
Uploaded to wrong dir
Uploaded to wrong dir
	modified:   CfdOF/CfdRemotePreferencePage.py
	modified:   CfdOF/Mesh/CfdMeshTools.py
	modified:   CfdOF/Mesh/TaskPanelCfdMesh.py
	modified:   Gui/CfdRemotePreferencePage.ui
	modified:   CfdOF/CfdRemotePreferencePage.py
	modified:   CfdOF/CfdTools.py
	modified:   CfdOF/Mesh/CfdMeshTools.py
	modified:   CfdOF/Mesh/TaskPanelCfdMesh.py
	modified:   CfdOF/Solve/CfdCaseWriterFoam.py
	modified:   CfdOF/Solve/TaskPanelCfdSolverControl.py
	modified:   Gui/TaskPanelCfdMesh.ui
@luzpaz
Copy link
Contributor

luzpaz commented Mar 20, 2023

Please add mention in the OP to #70

	modified:   CfdOF/CfdTools.py
	modified:   CfdOF/Solve/CfdCaseWriterFoam.py
	modified:   CfdOF/Mesh/TaskPanelCfdMesh.py
	modified:   CfdOF/Solve/CfdCaseWriterFoam.py
	modified:   CfdOF/Solve/TaskPanelCfdSolverControl.py
@oliveroxtoby
Copy link
Collaborator

oliveroxtoby commented Mar 24, 2023

@linuxguy123 I gave it a quick whirl and at first glance this looks fantastic!

A couple of initial wrinkles to report:

  • I had some problems using the preferences page for the first time. I couldn't quite figure it out but I seemed to need to enter the hostname and username, then save the host profile, then re-enter the hostname and username before 'Ping remote host' or 'Test SSH' would work without giving 'Error: missing hostname or IP address.'
  • It seems that the bashrc file isn't sourced on the remote machine before running the command; I suppose it is assumed to be loaded in bashrc at present? Could we change it to have this loaded like it is done locally? One can always use the pre-loaded environment used if no remote openfoam directory is specified, which is the current local behaviour.

@luzpaz
Copy link
Contributor

luzpaz commented Mar 27, 2023

@linuxguy123 just checking to see if you saw oliver's feedback above ?

@linuxguy123
Copy link
Author

Sorry, I've been busy. I'll have a look in the near future.

@linuxguy123
Copy link
Author

linuxguy123 commented Apr 5, 2023

@linuxguy123 I gave it a quick whirl and at first glance this looks fantastic!

Thanks ! I was beginning to think that only I would find it useful. I've been using it daily and I absolutely love it.

A couple of initial wrinkles to report:

I encourage feedback and discussion.

  • I had some problems using the preferences page for the first time. I couldn't quite figure it out but I seemed to need to enter the hostname and username, then save the host profile, then re-enter the hostname and username before 'Ping remote host' or 'Test SSH' would work without giving 'Error: missing hostname or IP address.'

Interesting. I should start with an empty instance and test that. Thanks for reporting this.

  • It seems that the bashrc file isn't sourced on the remote machine before running the command; I suppose it is assumed to be loaded in bashrc at present? Could we change it to have this loaded like it is done locally? One can always use the pre-loaded environment used if no remote openfoam directory is specified, which is the current local behaviour.

You are correct, the bashrc file isn't sourced on the remote machine. I mentioned in the user guide that remote processing assumes that OpenFOAM runs from the command line in the remote user's shell. What this means is that the user needs to add the OpenFOAM bashrc file to their remote .bashrc file.

I did this because executing stuff remotely (such as sourcing the OpenFOAM bashrc file) in an ssh session is sometimes non trivial. Knowing what I know now, I could probably source the OpenFAOM bashrc file when opening a shell, but it is way simpler if the user just adds it to the remote user's .bashrc file.

The other thing remote processing doesn't do is set up OpenFOAM on the remote computer. It is up to the user to do this manually and as such I didn't think that adding the OpenFOAM bashrc file to the remote .bashrc file would be a major inconvenience.

@linuxguy123
Copy link
Author

linuxguy123 commented Apr 5, 2023

I'm not happy with the code quality. As it sits now the code is functional but messy. Some of this is on purpose because I didn't want to mess with existing code thus I added code instead of integrating it into existing code. This caused duplication and also made things necessarily messy versus if RP was integrated into the existing structure.

My work stands as a decent functional prototype of what RP could be. If people think it is a worthwhile addition to CfdOF then we have 2 options:

  1. add it as is and accept the messiness.

  2. do a bit of a rewrite and properly integrate it into the existing CfdOF code. For instance, right now I use global vars to pass the host profile into the meshing and solving functions. Ideally the host profile would be passed in the mesh and solver objects.

I sunk a lot of time into this project and I'm happy I did. But I have to carefully allocate my time going forward as I am behind on my real project. It might be several months until I could do a proper rewrite by myself.

I also think that multiple developers are a good thing. I'd love to have someone else dig into my code and add their tweaks to it.

My needs for remote processing are satisfied for the time being with CfdOF-RP as it is. I look forward to direction from the community to decide our next steps.

@oliveroxtoby
Copy link
Collaborator

oliveroxtoby commented Apr 5, 2023

My work stands as a decent functional prototype of what RP could be. If people think it is a worthwhile addition to CfdOF then we have 2 options:

I definitely think it would be a worthwhile addition.

  1. add it as is and accept the messiness.
  2. do a bit of a rewrite and properly integrate it into the existing CfdOF code. For instance, right now I use global vars to pass the host profile into the meshing and solving functions. Ideally the host profile would be passed in the mesh and solver objects.

Personally I'm not overly worried about a bit of messiness in the code but would like to make sure the functionality is fairly polished.

I sunk a lot of time into this project and I'm happy I did. But I have to carefully allocate my time going forward as I am behind on my real project. It might be several months until I could do a proper rewrite by myself.

I would volunteer to dig in and file off the rough edges myself, but unfortunately my time is also very limited at the moment and I can't really see that happening.

Anyway, there is no hurry and you are welcome to come back to this at leisure when time permits.

@linuxguy123
Copy link
Author

Personally I'm not overly worried about a bit of messiness in the code but would like to make sure the functionality is fairly polished.

OK. Good to know. Test away then. I await feedback on things that needs polish.

I would volunteer to dig in and file off the rough edges myself, but unfortunately my time is also very limited at the moment and I can't really see that happening.

No promises but at some point in the future I may allocate an employee's time to working on CfdOF and FreeCAD in general.

Anyway, there is no hurry and you are welcome to come back to this at leisure when time permits.

OK.

@luzpaz
Copy link
Contributor

luzpaz commented Apr 6, 2023

I just hope this PR doesn't rot in the queue. 🤞

@linuxguy123
Copy link
Author

I just hope this PR doesn't rot in the queue. crossed_fingers

Users can use CfdOF-RP right now via settings in Tools->Addon Manager Options

CfdOF in Addon Manager

I have pulled and will pull the recent changes from CfdOF into CfdOF-RP so it is current with Master. It could be merged back into CfdOF any time, but I really want people to get some time on it before we do that.

I have every intention of merging back into CfdOF. While I forked CfdOF to do this work, it is not my intention to fork CfdOF.

@linuxguy123
Copy link
Author

linuxguy123 commented Apr 7, 2023

The CfdOF-RP repo should be sync'd with CfdOF Master now. I'm not sure why it is saying that the branch cannot be rebased due to conflicts. I'll look into it next week.

22 commits ahead

@luzpaz
Copy link
Contributor

luzpaz commented Apr 18, 2023

@oliveroxtoby perhaps this PR can be merged in to a experimental branch and then testers can use the FreeCAD Addon Manager feature to toggle on the experimental branch ?

@oliveroxtoby
Copy link
Collaborator

@luzpaz done, and merged latest master in branch linuxguy-remote-processing. Feel free to communicate this as you see fit.

@oliveroxtoby
Copy link
Collaborator

A couple of initial wrinkles to report:

I encourage feedback and discussion.

  • I had some problems using the preferences page for the first time. I couldn't quite figure it out but I seemed to need to enter the hostname and username, then save the host profile, then re-enter the hostname and username before 'Ping remote host' or 'Test SSH' would work without giving 'Error: missing hostname or IP address.'

Interesting. I should start with an empty instance and test that. Thanks for reporting this.

Hi @linuxguy123, just wondering if you'd be willing to look at this issue?

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.

3 participants