sassoftware / saspy

A Python interface module to the SAS System. It works with Linux, Windows, and Mainframe SAS as well as with SAS in Viya.
https://sassoftware.github.io/saspy
Other
373 stars 150 forks source link

sd2df didn't raise an Error if the table doesn't exist #425

Closed Debbby57 closed 2 years ago

Debbby57 commented 2 years ago

Is your feature request related to a problem? Please describe. Hello !

I have the code below :

with my_session as sas:
      sas.submit("data my_table; set my_source;run;")
      my_table_df = sas.sd2df("my_table");
      if sas.SYSERRORTEXT():
          raise Exception("SAS LOG ERRORS : " + sas.SYSERRORTEXT())

If I make a mistake in the sas submitted code (for example," my_source" doesn't exist so I can't create "my_table"), I have a SAS Code Error.
But if I make a mistake in the SAS Tablename when I use the sd2df function (for example, "my_table" doesn't exist), SAS doesn't return a code Error. The log indicates The SAS Data Set work.my_table does not exist but the sas.SYSERR() returns 0 and the sas.SYSERRORTEXT() is empty.

Describe the solution you'd like I would like to have a SAS Error because I may not use my_table_df immediatly but I prefer to stop the python script immediatly since it's going to crash anyway.

Thank you :)

tomweber-sas commented 2 years ago

Hey @Debbby57 sorry I wasn't able to get to this today. You are correct that sd2df() with a non-existent table won't cause the SYSERRORTEXT auto macro to have an error in it. That's because saspy checks to see if the table exists before trying to access it and if it doesn't exist, it returns None, along with the error message that the table doesn't exist. So, there's no code submitted to SAS that would fail and generate an error in the macro variable. There are a couple ways you can programmatically check for this kind of issue in your code already, so that you can return your own error. You can use the exist() method to see if the table exists or not (same as sd2df does under the covers) or you could check the returned variable from sd2df, which is a SASdata object if successful, or None if not (the case here). The SAS auto macros, like SYSERRORTEXT are not populated by saspy, but rather SAS, so for things where saspy catches a problem and never submits code to SAS which would error, those macro variables aren't populated since there wasn't a SAS error (no code ran that caused an error).

Here's some example code you could integrate into what you have above; use exist() to see if the table you're trying to access exists, or check what you get back from sd2df to see if it worked or not:

>>> df = sas.sd2df('tom')
The SAS Data Set .tom does not exist
>>> df
>>> type(df)
<class 'NoneType'>
>>> df is None
True
>>> sas.SYSERRORTEXT()
''
>>> sas.exist('tom')
False
>>>

So, for instance:

with my_session as sas:
      table = 'mytable'
      sas.submit("data {}; set my_source; run;".format(table))
      my_table_df = sas.sd2df(table);
      if sas.SYSERRORTEXT() or not sas.exist(table):
          raise Exception("SAS LOG ERRORS : " + sas.SYSERRORTEXT())

or you could do:

      if sas.SYSERRORTEXT() or my_table_df is None:
          raise Exception("SAS LOG ERRORS : " + sas.SYSERRORTEXT())

Both of those will return the exception you have coded.

Is that a reasonable way for you to programmatically check for this kind of situation? You can't only count on the SAS macro variables for all cases as they are only for errors that happen in SAS itself.

Thanks, Tom

Galileo-Galilei commented 2 years ago

Hi Tom,

Thank you for above explanation. I am concerned by this issue too, and I strongly feel that saspy should return an error in the use case described by @Debbby57.

I understand that SYSERRORTEXT and SYSERR are populated by SAS itself (not saspy) and will be empty when sas.sd2df("fake_table")" is not found. However and for consistency with other python popular packages API, I think that this line of code should raise a python error.

For comparison sake, pandas raises an error if the path does not exist:

import pandas as pd
df=pd.read_csv("fake/path/to/file.csv") # return a `FileNotFoundError` if the file does not exist

I feel raising an error is the natural behaviour expected for a file reader in python, and saspy would benefit to be consistent with other python popular packages API (I really can't find a use case where I would prefer having an "None" object instead of an error: if I am reading a file, I do expect it to exist before I can continue running my code!).

Catching the error on my side is possible, but I have big pipelines running and I should do it for each table which is not very user friendly and error prone. Moreover, if I forgot to catch this "None" value somewhere, the pipeline sometimes continue running a long time (reading other tables) before crashing (once I finally try to use the faulty table).

Would you mind considering changing sd2df behaviour to raise a python error in this situation (maybe a TableNotFoundError), or do you have any specific use case to prefer returning None over raising an error?

tomweber-sas commented 2 years ago

Hey @Galileo-Galilei , those are very good points. You make a compelling case. I will look into raising an exception for this and get back to you all on this thread. I don't like to change behavior, as that can break backward compatibility, though in this case, it is really just tightening up an error case, as you mention, where you get a direct failure instead of an indirect one later (if you didn't check the return type). So let me investigate and see what I see.

Thanks! Tom

Galileo-Galilei commented 2 years ago

Thank you very much for considering this option! I was afraid that breaking backwards compatibility would be an issue for you, but I really think this can be considered as a bug fix rather than a functional breaking change (I do not think users reasonably assume that a non existing table will return None). I'll wait for your feedback then :)

tomweber-sas commented 2 years ago

I'm always open to suggestions, for changes and improvements! Backwards compatibility is the primary concern for something like this though. The change is a one liner, so that's not a problem. I can see justifying the change as a 'fix' for this case, but I'm a little concerned about opening a can-o-worms. To be perfectly transparent, there's probably dozens of other places where something similar could be requested, and changing all of them would be too much w.r.t. breaking existing code in the field.

When I first wrote this, 6 years ago or so, I came at it from a return code/return value point of view for programmatically checking for errors (I was an Assembler/C programmer). So throwing exceptions (abending your program), wasn't how I went about a lot of cases. I didn't even know python when I wrote this (had to teach myself the language), so the python/java way of throwing exceptions for everything, and the user having to try/catch/except, wasn't how coded a lot of things; I checked return code/values and decided what to do - throw an exception if that's what you want or just handle the case, however you would. But, clearly, the Pythonic way is to throw exceptions.

I have no problem with that, but that's not how I wrote it all back then. I've enhanced it to do more exception over time, and again, think this is ok for the most part, but I do take backwards compatibility seriously and thus have to weigh being more pythonic now, with breaking changes.

I'll push this change out to main, and you both can try it out see it's it's as you expect. Probably tomorrow. That other caveat is that I'm going to be out the next 2 weeks, w/out the ability to access this. So. it'll be not till then that I'll be able to get back you you on this.

Thanks! Tom

Debbby57 commented 2 years ago

Hello Tom,

Thank you very much for your help ! I'm going to try this changes when they will be in the main.

tomweber-sas commented 2 years ago

Hey, sorry this took so long. Had 10 fires yesterday and today, of course since I'm going to be w/out access to things for a couple weeks. But, I pushed this change to a branch named 'exception' (instead of main), so you can pull that and try it out. I won't be able to put enough time into this till I'm back to really assess if this one change is ok, or if it begs the question of a number of other places the same kind of then should then be done; and if so, is this, or all of those too big of a breaking change. If so what then ... I'll figure out that later But, in the meantime, you can at least see what you think about this change itself.

Thanks! Tom

Debbby57 commented 2 years ago

Hi Tom,

I tested the exception branch and I obtained a beautiful Python Error FileNotFoundError: The SAS Data Set work.stock does not exist

The solution is perfect for me ! :smiley:

Thank you

tomweber-sas commented 2 years ago

Thanks for the confirmation @Debbby57. I'm back from vacation, and have been dealing w/ other fires (log4j blowup!) and others. I'll try to get back on this issue for y'all. I'll need to refresh myself on this and get a better idea of is this is more pervasive than this one place in the code, or not. Just wanted to let you know I'm back and will be looking into it :) Thanks, Tom

tomweber-sas commented 2 years ago

Hey y'all, sorry this is taking so long. Just the wrong time w/ me taking vacation and the SAS holiday all adding up. But I'm back and trying to get back to things; hope you all had a good new year.

I looked through my code and I see a number of places where anyone could argue that I should be throwing an exception as opposed to returning, for lack of a better term, a return code (None, or some other thing to check). That's what I was afraid of, in terms of making those changes and breaking existing code. But, what I'm thinking is to add a config option (like I've done for other things like this), so you just set it in the config and then the behavior is what you want instead of the default, which stays the same.

Thus then the dilemma of scope. I certainly don't want an option per occurrence of a breaking change. So what I'm thinking is an open ended option for this category of change; Accept Pythonic Breaking Changes, APBC=True. Or whatever the name. The point being that as I have to tweak code over time which causes a breaking change (having to do which things like this), you accept future breaking changes (in this category) and accept you may have to tweak your code to account for them as they show up on occasion, in a future release.

Or, I could have a versioning system where you say the version you want the behavior to exhibit (again, for these kinds of changes). That allows you to 'lock in' a given version, so a future breaking change doesn't bother you, but you can make the change later (if you like it) and then up the version to the version that change was in; after you've coded the change. This seems a little more generic and could be used to address any breaking change, really. I'm just thinking this up now :) but it seems like a better idea. More complication on my end, but that's what software's supposed to be for, do the hard work and make it easy on the user.

Walking through this idea would be like the following. The current version is 3.7.8 (and that would be the default), so if I added this one change you want, then it wouldn't throw the exception unless you set BCV=3.7.9 or higher (Backward Compatibility Version is what that one stands for). If I make another breaking change in a higher version, it wouldn't affect you unless you bumped the version to that number, or higher of course. If you want every breaking change as they come, set it to 10.0.0 and that's always higher than the current version; as long as I'm alive and working on it I expect :) haha or 999.999.999 if you like.

This way you won't be hit by breaking changes, but you can accept them at will, as you make the changes in your code. At least that's what I'm thinking. What are your thoughts on this? Trying to address this one, and future other changes, with one simple implementation (from a users point of view).

Thanks, Tom

tomweber-sas commented 2 years ago

So, following up on this thought, I've coded it up and I think it has legs. I've pushed the change to the 'exception' branch. What I've added is the ability to specify SAS_BCV = '3.7.8' (or whatever version you want, and BCV can be Backward Compatibility Version, or Breaking Change Version :) haha), either in the config file (on its own, not inside a config definition), or override it on the SASsession(..., SAS_BCV='3.7.9') so it's easy to configure as well as test on the fly to see if your code's been affected by any changes, without having to change your config file from it's setting.

So, in my config I can have added it (not my real file, just a sample so you see where it goes):

SAS_config_names = ['default']

SAS_config_options = {'lock_down': False,
                      'verbose'  : True,
                      'prompt'   : True
                     }
#SAS_output_options = {'output' : 'html5'}

SAS_BCV = '3.7.9'

default  = {'saspath': '/opt/sasinside/SASHome/SASFoundation/9.4/bin/sas_u8',
            'options' : ["-fullstimer"]
            }

And given that, if I pushed this change to V3.7.9 (3.7.8 is current), then you'd get the exception for file not found. If you didn't specify this or it was lower than 3.7.9, you would still get the message and your df object returned as None.

You can provide a value on the fly on SASsession() to see what the effect is for a different version, if any. Below I have it set in my config to 3.7.9, fyi.

tom64-5> python3.5
Python 3.5.6 (default, Nov 16 2018, 15:50:39)
[GCC 4.4.7 20120313 (Red Hat 4.4.7-23)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import saspy
>>> sas = saspy.SASsession(cfgname='sdssas'); df = sas.sd2df('tom', 'sashelp'); df is None
SAS Connection established. Subprocess id is 12777

The SAS Data Set sashelp.tom does not exist
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/tom/github/saspy/saspy/sasbase.py", line 1569, in sd2df
    return self.sasdata2dataframe(table, libref, dsopts, method, **kwargs)
  File "/opt/tom/github/saspy/saspy/sasbase.py", line 1745, in sasdata2dataframe
    raise FileNotFoundError('The SAS Data Set ' + libref + '.' + table + ' does not exist')
FileNotFoundError: The SAS Data Set sashelp.tom does not exist
>>>
>>>
>>> sas = saspy.SASsession(cfgname='sdssas',SAS_BCV = '3.6.9'); df = sas.sd2df('tom', 'sashelp'); df is None
SAS Connection established. Subprocess id is 12829

The SAS Data Set sashelp.tom does not exist
True
>>>
>>>
>>> sas = saspy.SASsession(cfgname='sdssas',SAS_BCV = '3.16.9'); df = sas.sd2df('tom', 'sashelp'); df is None
SAS Connection established. Subprocess id is 12881

The SAS Data Set sashelp.tom does not exist
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/tom/github/saspy/saspy/sasbase.py", line 1569, in sd2df
    return self.sasdata2dataframe(table, libref, dsopts, method, **kwargs)
  File "/opt/tom/github/saspy/saspy/sasbase.py", line 1745, in sasdata2dataframe
    raise FileNotFoundError('The SAS Data Set ' + libref + '.' + table + ' does not exist')
FileNotFoundError: The SAS Data Set sashelp.tom does not exist
>>>
>>>
>>> sas = saspy.SASsession(cfgname='sdssas',SAS_BCV = '3.7.8'); df = sas.sd2df('tom', 'sashelp'); df is None
SAS Connection established. Subprocess id is 12931

The SAS Data Set sashelp.tom does not exist
True
>>>

Let me know what you think about this design. I think it's a easy way for me to be able to introduce breaking changes in a way that users won't be affected by until they want to make the corresponding changes in their code. And it's an easy way to see if there even are any breaking changes in your code, on the fly w/out changing your config, for a newer version, without getting 'bit' by them until you're ready to make a change to your code (if you ever want to).

Thanks, Tom

Debbby57 commented 2 years ago

Hello Tom

Thanks for your reply et sorry for the late answer !

I'm not sure to understand all your proposition. This new configuration parameter concern only breaking changes isn't it ? So if you push à 3.7.9 version I install it but if I write 3.7.8 in the SAS_BCV parameter some codes/functions will be on 3.7.8 version ? I find this a little muddled (I'm not sure of the correct translation sorry) : you use a package version but some part of the package uses old code. Moreover, suppose, in the future, I don't want the 3.7.9 changes but I want the hypothetic change in 3.8.5, I can't do that. So Finally I would have to accept the 3.7.9 changes (it's just I would have to do it later)

I have some questions :

tomweber-sas commented 2 years ago

Hey @debbby57 thanks for the response and the questions. I think you have the first parts right. The idea of this versioning is only for breaking changes. I think I’ve only made one of these in 6 years, so I don’t expect this to be something that’s used a lot. But, it would allow the introduction of breaking changes (on occasion; maybe more than 1 in 6 years, haha), without any of the need for what you were describing in the last point about semver, for your install: all the work you have to do to ever use a newer version that perhaps has something you want, but you have to deal with having to update all your programs before you can install that newer version. This allows you to get the new version and do those updates, if you want to, after the fact. It’s not so much about old code, but simply the behavior of your program, as it’s coded, for a case where that existing behavior changes out from under you.

As for is this a breaking change, well yes it is. I can’t make an assumption that no one ever conditionally handles a case where a file doesn’t exist, even if it’s expected to. To say all programs should fail at that point isn’t realistic. Simply given the fact that this one method returning a None object for cases where it failed to complete successfully, for any number of reasons, means there can be code out there that checks for that and does something, continuing on. Now, they would have to rewrite their code to use try: except: instead of if: else: otherwise their code would now fail when it wasn’t previously. So, it’s a breaking change, just by definition. I can’t really speculate how many programs may or may not code something a certain way. So, I have to consider it' breaking someone’s code, by my standards.

The other thing you mentioned about it being incrementally inclusive; each newer version means you get all the previous changes. Yes, that is true, because trying to cherry pick which changes would just be way overly complicated. I think your description of the semver, where you can only have specific versions installed is a good example of how this enhances that, really. In the semver case, you describe that in order to use a newer version, you have to first, go study all the breaking changes and then go make all of the necessary changes to all your programs before you can upgrade to a new version; all of them up to this new version (not just random changes, that’s incrementally inclusive too). What this version specification gets you is that you can always install the latest version with whatever fixes or new functionality, or whatever, without having to change all your code for any breaking changes that may have also happened, before using the new version. You can study the changes after the fact, and make some of them, but not all of them, if you want (yes, it has to be incremental from the early releases toward the newer ones). But you don’t actually ever have to change your code to get the same behavior, while still getting any fixes or performance enhancements or whatever benefits the newer version has.

Another thing this provides is the ability to have different programs using different levels of code against the same installed version of saspy. You may have ‘legacy’ code that you never want to go have to rework for things like this, while newer code, you would want to make changes to. You get this by default now, and in future programs, after some of these changes have been introduced, you can 'checkpoint' the behavior for them, such that they never get broken in future releases either by specifying the version they are at in the SASsession() method in those programs (or the config file they use), and that code isn’t effected by any future breaking code changes regardless of which SASPy version it’s run against over time (saspy being updated won’t break it yet it gets any improvements).

If you want usual breaking change behavior (it breaks you if you don’t change your code first) just set the option to 999.999.999 in your config, and that then get’s you how it would all work today if I made breaking changes; to upgrade, you have to always make any required changes to all your programs before you can install a newer version, else you programs may break. But anyone who doesn’t want their code broken, yet wants the latest version, gets that by default without doing anything (don't even need the new option).

This probably sounds overly complicated, but I think it’s really a simple thing that would allow me to make breaking changes that some people want, instead of not making them, while at the same time, not breaking others’ existing code.

Sorry, that got long. But I’m trying to get all the benefits I see, explained to see what my reasoning is for doing something like this. I also get that this isn’t ‘how everyone else does it’, but I think it can be better then that usual case. I generally want users to use the latest version of saspy at all times. Again, I can’t control any of that, but not making breaking changes allows that to be viable. You can upgrade to the latest, right now, without having any of your programs break. This allows for fixes and performance enhancements, ..., that everyone can have immediately without any work on their side. So to me, this version idea allows for introducing breaking changes (that I would otherwise likely not do), while at the same time, still allowing for this ‘always newest without breaking existing code’ behavior which I think is a significantly important aspect.

So, what do you think, if you understand all of that. I hope I made all of that clear. I really like the discussion and want to end up with the best functionality for everyone. @Galileo-Galilei , thoughts?

Tom

Galileo-Galilei commented 2 years ago

Hi Tom, first of all thank you very much for the time and efforts you're putting in!

To be honest, my personal thoughts on the subject are mitigated:

As a conclusion: i'd personnally be inclined to follow SemVer and publish some breaking changes if it clear in the CHANGELOG and the version number, but I understand that retrocompatibility is an absolute feature for you (after all SAS is a paid software and people have licences of different versions, so I understand this library focuses on being compatible with as many customers as possible), so your solution is sensible and totally acceptable for my use cases.

tomweber-sas commented 2 years ago

@Galileo-Galilei (and @Debbby57), again thanks for your insight and thoughts and opinions on this. I am taking in all of this, and doing a little research on various details, and trying to incorporate my specific vision and/or requirements to then have a (more concise than usual) response to this :) Sometimes I need to let things churn as a background task in my head to come up with insight. I want to have both here; do it the open source/python way, yet also provide my users what the open source way doesn't provide. So, just wanted to acknowledge you post(s) right away, but I need a little more time to research this and think about it to provide an actual response (I have a lot in my head but don't want to brain dump here). So, thanks!, and I'll reply soon ...

tomweber-sas commented 2 years ago

Ok, I've had a chance to ponder all of this (had to write it all down for myself; I'll post that below, but provide the short answers here so you don't have to read it all).

I agree that what I should be doing regarding semver and breaking changes is the OpenSource way. So, I will push this change; throw exception instead of return None. I will then explicitly identify this breaking change in the release notes (~changelog), and bump the Major version to 4 to identify such.

I will also support having the ability to provide a BCV release to allow for the previous behavior, as a convenience to any of my users who need to use that until they can make appropriate changes (if ever). The default will be that breaking changes are enabled, so nothing has to be done, and it will be the expected open source way. I also agree that supporting BCV could become a hassle and get complicated, so I'm not going to doc that or truly commit to it yet, till I see if it's all in my head an no one cares, or it turns out to be a handy special case tool for some. Either way, it's not the default case, only a possible exception if needed. So the normal way will be as you would expect (you never need to have this option).

I believe this is acceptable to both you, based upon previous statements, so I'm going to merge this exception branch into main, and then I have to update the log4j jars to the dot release versions they have today, that are supposedly ok. Once I get all of that done and Document the breaking changes, I'll be read to build the new 4.0.0 release and get that out on pypi and conda as well.

Sorry this has taken so long. But it's been a good endeavor for me; getting this breaking change deal figured out, since it is inevitable. Just hadn't really had to figure it out before. Thanks again for all of your help and input on this!

Here's my rational (or whatever) just FWIF, that helped me try to put it all in perspective.

1) open source vs. proprietary software I have one foot on each side of this fence. And I have users across the spectrum. SASPy is an open source, python source code, user contributable, GitHub, free package. It’s only real use is to interface w/ a proprietary, paid for, won’t break your production programs when you upgrade to a newer version to get new functionality, fixes or enhancements software system. I have users who don’t know Python, users who don’t know SAS, some who know neither and some both. Many users know nothing about these open source standards or that they have to go change their code when the major version increments. I strive to support and honor all of these combinations. Thus my dilemma.

2) semantic versioning (semver) SASPy has a semantic version; Major.Minor.Patch (3.7.8 is current). I have not followed the strict rules about incrementing, so far. I have honored the magnitude that each of the 3 levels embrace. But I’ve done more of what’s considered serial versioning with it (more like an odometer). The 3 major versions I’ve used (1,2,3) weren’t because of breaking changes, but rather major enhancements that seemed worthy of the highest magnitude change in the version number. I will start following the semantic version rules from now on; including updating MAJOR for any breaking changes.

3) changelog I don’t have a changelog file. I’ve been using the release notes to identify the changes in each version. This is perhaps, close enough. I see other repos w/out the changelog file, and some that have one and its's nothing more than just the release notes themselves concatenated into a file. So, I guess it’s all kinda the same. I will explicitly doc any breaking changes, regardless.

4) backwards compatibility This is a stickler for me. On the proprietary side of the fence, I can’t be breaking existing code, or force users to go through all of their programs and change things, just to be allowed to use a newer version with fixes and enhancements. That’s unacceptable to me. So, this breaking change thing is an issue. I want (maybe even need) to be able to roll out breaking changes though, because making some things more opensource/Pythonic will cause this kind of change. So I have to do both; make breaking changes for the Python/opensource side of the fence, yet not break production code for the Proprietary side of the fence. Thus my dilemma.

5) BCV option (Backward Compatibility Version or Breaking Change Version) The BCV option idea I’m proposing allows for having both things to be able to exist. Unconventional as it is, it’s not Pythonic. But, it provides users a way to use newer versions of SASPy, with breaking changes, without having to (sure they should when they can) change their existing programs. I see how this can seem complicated and cause confusion, as opposed to just saying fix all your code or don’t use a new release; but again, that’s unacceptable to me. If however, I switch the default from ‘you have to add an option to get the new behavior’ (failsafe case to not break existing code), to ’you get the new breaking change’ (identified by the major number incrementing - standard Opensource operating procedure), but you 'can' set a BCV to an earlier version and then your code will still behave as before any breaking changes, then maybe that will allow for both. Make the default the usual open source way that things work, but I provide an override so you can use newer SASPy versions with older programs that you don’t have to rework (optional failsafe you can use when necessary). Is that a good compromise to get both?

Galileo-Galilei commented 2 years ago

Hi Tom, than you very much for your dedication time and excellent support!

Debbby57 commented 2 years ago

Hello Tom !

thank you for your message and great explantations.

I am in tune with your solution which makes everybody happy :) Your solution allows you to follow SemVer and on the other hand you still have retrocompatibility thanks to your BCV option. I agree that it's an excellent compromise !

As I am concerned for the changelog file, I think it's very userful for users to have it but like you said all of python projects don't have it. What I absolutly want is the info of the changes so as long as there are at the minimum in the release notes it's good for me

tomweber-sas commented 2 years ago

Thanks again for both of your feedback. I will add a CHANGELOG.md to my repo; trying to figure out the formatting now :) I'm not going to retro add the previous 59 releases to it, but I'll add to it from now on for future releases. Starting at an X.0.0 semver seems like an ok start, I suppose.

I've got the latest log4j jars and all of that updated, so I have to run tests and finish up the changelog and release notes. I'm hoping to get this into production this afternoon!

Thanks, Tom

tomweber-sas commented 2 years ago

I created release V4.0.0 (Pypi and Conda too) with these changes and everything we've talked about here. Also updated the log4j jars. I'm going to close this issue, but if either of you have any other concerns, just reopen or open a new one. Again, thanks for all the input! Tom