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

Formalize ROSS API #162

Open
gonsie opened this issue Sep 18, 2019 · 3 comments
Open

Formalize ROSS API #162

gonsie opened this issue Sep 18, 2019 · 3 comments

Comments

@gonsie
Copy link
Member

gonsie commented Sep 18, 2019

The ROSS API isn't very well documented. Models are expected to manually set several g_tw_xx variables, all of which are exposed through the header files... which means that an installation of ROSS must install several header files.

In my dream world, only ross.h is installed and the API to configure ROSS is consistent and clearly documented.

Thoughts from @markplagge and @nmcglohon (or others from codes-org)?

I think the way to make progress on this would be:

  1. figure out which global variables models are using.
  2. create an API to allow models to set various options.
  3. clean up ross.h. Maybe create a new ross-internal.h for ross itself and use ross.h as the one for models to include?
This was referenced Sep 18, 2019
@caitlinross
Copy link
Member

Yeah I think this would be a good idea. It would also be useful for making improvements in ROSS without affecting models. e.g., when removing the remnants of pthread ROSS, g_tw_pe used to be an array of PE pointers, so I changed it. Though of course that required a change in CODES because in their mapping set up, they accessed that PE array. We should be able to make changes in ROSS like that without being able to affect models.

On that note, I know for sure that g_tw_pe is used in CODES. :) And g_tw_ts_end. I'm sure there's more but I know those off the top of my head.

@markplagge
Copy link
Collaborator

I think this would be a great idea. Having a single ross.h file to include which contains the components that models should be using is a great idea. Making ROSS more stand-alone library like is a good thing.

Moving to something like this is pretty challenging, especially without breaking backwards compatibility. I know that NEMO uses the global LP array, but that is being removed. The new code-base for NeMo actually uses the tw_pe function hooks too.

I like number 3, that would make things simpler as it would be obvious which ROSS functions are okay to use, and which would be risky for a model to use.

@nmcglo
Copy link
Member

nmcglo commented Sep 27, 2019

Sounds good to me, I'm a big fan of abstracting away stuff like that. That way future changes don't require such broad changes across CODES.

My only request would be to have some warning before it's merged to master. Keeping CODES master always compatible with latest ROSS master is my goal as when that doesn't work I start to get emails 😂 so synchronizing an API change would be preferable for me.

This all being said, CODES uses g_tw_* values A TON. About 277 times. A lot of g_tw_ts_end, g_tw_lookahead, g_tw_mynode is the majority. I can make an exhaustive list though.

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

No branches or pull requests

4 participants